Implement shulker boxes#20
Open
HashimTheArab wants to merge 59 commits into
Open
Conversation
Implemented inventory Implemented sounds Implemented opening/closing animations
Hope it doesn't break anything else
…nfly into feature/shulker-box Sync local repositry with remote repositry
Co-authored-by: DaPigGuy <mcpepig123@gmail.com>
Merge master into feature/shulker-box to fix any possible PR merge conflicts
Reference: 9cb575e
Co-authored-by: RestartFU <45609733+RestartFU@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis pull request implements a complete shulker box container block with an animated lid, entity displacement mechanics, and full NBT persistence. The changes introduce an OptionalColour type system, extend displacement handling across player/entity/handler interfaces, and integrate shulker boxes into the block registry and container system. ChangesShulker Box Feature
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
server/item/optional_colour.go (1)
41-45: ⚡ Quick winAvoid hardcoding optional-colour count.
Line 42 hardcodes
17, which couples this helper to the current dye count. Derive fromlen(colours)to keep it future-proof.Proposed fix
- optionalColours := make([]OptionalColour, 17) + optionalColours := make([]OptionalColour, len(colours)+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 `@server/item/optional_colour.go` around lines 41 - 45, The slice length in OptionalColours is hardcoded to 17; change it to derive from the colours slice so it stays correct when dyes change: allocate optionalColours with length len(colours)+1 (since the loop writes at index i+1), then populate with NewOptionalColour(c) in the existing loop over colours; update OptionalColours, referencing the colours variable and NewOptionalColour accordingly.server/block/shulker_box.go (2)
167-184: 💤 Low valueUse the return value of
Addand avoid the redundant trailing tick.
progress.Add(1)/Add(-1)already return the new value, so the extraLoad()is unnecessary. Also, after the state transitions toshulkerStateOpened/shulkerStateClosedin this branch, the unconditionalScheduleBlockUpdateresults in one wasted tick that just re-stores the boundary value before settling. You can scope the reschedule to the still-animating case.♻️ Proposed cleanup
case shulkerStateOpening: - s.progress.Add(1) + p := s.progress.Add(1) s.pushEntities(pos, tx) - if s.progress.Load() >= shulkerLidTicks { + if p >= shulkerLidTicks { s.progress.Store(shulkerLidTicks) s.animationStatus.Store(shulkerStateOpened) + return } tx.ScheduleBlockUpdate(pos, s, 0) case shulkerStateOpened: s.progress.Store(shulkerLidTicks) case shulkerStateClosing: - s.progress.Add(-1) - if s.progress.Load() <= 0 { + p := s.progress.Add(-1) + if p <= 0 { tx.PlaySound(pos.Vec3Centre(), sound.ShulkerBoxClose{}) s.progress.Store(0) s.animationStatus.Store(shulkerStateClosed) + return } tx.ScheduleBlockUpdate(pos, s, 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 `@server/block/shulker_box.go` around lines 167 - 184, In shulkerStateOpening and shulkerStateClosing use the return value of s.progress.Add(...) instead of calling s.progress.Load() again, and only call tx.ScheduleBlockUpdate(pos, s, 0) when the new progress value is strictly between 0 and shulkerLidTicks (i.e., still animating); when progress reaches the boundaries, set s.progress.Store(0 or shulkerLidTicks), update s.animationStatus to shulkerStateOpened/shulkerStateClosed, play the close sound in the closing branch as now, and avoid the unconditional ScheduleBlockUpdate that causes a redundant tick. Ensure you reference s.progress.Add, shulkerLidTicks, shulkerStateOpened, shulkerStateClosed, shulkerStateOpening, shulkerStateClosing, tx.ScheduleBlockUpdate, and sound.ShulkerBoxClose when making the changes.
188-198: 💤 Low valueRemove the [0] index concern and clarify the 0.35 growth value.
The
BBoxmethod returns a single-element slice, so the[0]index is always safe—not an assumption. However, the0.35growth value merits documentation: it appears to be an intentional margin for entity hitboxes around the extended lid (which itself reaches 0 to 0.5 blocks). Consider extracting this as a named constant (e.g.,shulkerPushMargin) to clarify intent.🤖 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/block/shulker_box.go` around lines 188 - 198, Replace the magic 0.35 literal with a named constant and make the single-element BBox usage explicit: define a file-level constant shulkerPushMargin (e.g., const shulkerPushMargin = 0.35) with a short comment explaining it’s the extra margin to cover entity hitboxes around the extended lid, then in pushEntities use the single returned bounding box explicitly (bbox := s.Model().BBox(pos, tx)[0]) and call bbox.Translate(pos.Vec3()).Grow(shulkerPushMargin) so the intent and safety of the single-element BBox and the margin are clear.
🤖 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 `@server/block/model/shulker.go`:
- Around line 27-30: The ShulkerPhysicalPeak function computes a peak from an
unvalidated progress value; clamp the input progress to the valid range [0,10]
before computing t so out-of-range values cannot produce negative or >0.5
results. Modify ShulkerPhysicalPeak to bound the int32 progress (e.g., if
progress < 0 set to 0, if progress > 10 set to 10) and then convert the clamped
value to float64 for the existing formula, keeping the rest of the computation
in the same function.
In `@server/block/shulker_box.go`:
- Around line 221-234: The current Y-axis logic skips downward-facing shulkers
(when s.Facing.Offset().Y() == -1) so entities touching a ceiling-mounted
shulker aren't moved; add a branch for offset.Y() < 0 that mirrors the upward
case but pushes entities downward so their top is at or below
shulkerBBox.Min().Y(). Specifically, in the function using s.Facing.Offset(),
entityPos := e.Position(), shulkerBBox and mover.Displace, compute targetY :=
shulkerBBox.Min().Y() for the downward case and call mover.Displace with a
negative Y displacement when entityPos.Y() (adjusted for whether entityPos
represents feet or center in this codebase) is above targetY; keep the existing
early-returns for the other Y cases.
- Around line 62-72: The inventory currently suppresses viewer updates for
ShulkerBox items but still accepts them server-side; update the inventory
creation for s.inventory (the inventory.New callback) to instead install a slot
validator that rejects ShulkerBox placements by calling
s.inventory.SlotValidatorFunc and returning false when item.Item() is a
ShulkerBox, and keep the existing viewer notification loop (which calls
viewer.ViewSlotChange) unchanged so clients and server stay in sync.
- Around line 205-210: The current logic checks for the Displace method on e
(mover, ok := e.(interface { Displace(deltaPos mgl64.Vec3, deltaYaw, deltaPitch
float64) })) and returns for any entity that doesn't implement it, causing
non-player entities to be ignored; fix this by ensuring all pushable entity
types implement Displace (or add Displace to the common entity base/interface)
so mobs, projectiles, and items are handled, or change the shulker-box
displacement path to fall back when ok==false by applying movement via a shared
API (e.g., a generic Translate/SetPosition or ApplyVelocity method) using the
same deltaPos (mgl64.Vec3) and rotation deltas; update the implementations for
entity types that lack Displace (or add the fallback branch) so no entity is
skipped.
In `@server/item/optional_colour.go`:
- Around line 20-37: The Colour() and Prepend() methods on OptionalColour
currently index colours[(oc-1)] without checking an upper bound; add a bounds
check in both OptionalColour.Colour() and OptionalColour.Prepend() to ensure (oc
> 0) and (int(oc-1) < len(colours)) before indexing, returning (Colour{}, false)
from Colour() and the original str from Prepend() when the value is out of range
to avoid panics from malformed input.
---
Nitpick comments:
In `@server/block/shulker_box.go`:
- Around line 167-184: In shulkerStateOpening and shulkerStateClosing use the
return value of s.progress.Add(...) instead of calling s.progress.Load() again,
and only call tx.ScheduleBlockUpdate(pos, s, 0) when the new progress value is
strictly between 0 and shulkerLidTicks (i.e., still animating); when progress
reaches the boundaries, set s.progress.Store(0 or shulkerLidTicks), update
s.animationStatus to shulkerStateOpened/shulkerStateClosed, play the close sound
in the closing branch as now, and avoid the unconditional ScheduleBlockUpdate
that causes a redundant tick. Ensure you reference s.progress.Add,
shulkerLidTicks, shulkerStateOpened, shulkerStateClosed, shulkerStateOpening,
shulkerStateClosing, tx.ScheduleBlockUpdate, and sound.ShulkerBoxClose when
making the changes.
- Around line 188-198: Replace the magic 0.35 literal with a named constant and
make the single-element BBox usage explicit: define a file-level constant
shulkerPushMargin (e.g., const shulkerPushMargin = 0.35) with a short comment
explaining it’s the extra margin to cover entity hitboxes around the extended
lid, then in pushEntities use the single returned bounding box explicitly (bbox
:= s.Model().BBox(pos, tx)[0]) and call
bbox.Translate(pos.Vec3()).Grow(shulkerPushMargin) so the intent and safety of
the single-element BBox and the margin are clear.
In `@server/item/optional_colour.go`:
- Around line 41-45: The slice length in OptionalColours is hardcoded to 17;
change it to derive from the colours slice so it stays correct when dyes change:
allocate optionalColours with length len(colours)+1 (since the loop writes at
index i+1), then populate with NewOptionalColour(c) in the existing loop over
colours; update OptionalColours, referencing the colours variable and
NewOptionalColour accordingly.
🪄 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: fc065ec6-85aa-46f4-b872-45aa53834ace
📒 Files selected for processing (18)
cmd/blockhash/main.goserver/block/cube/face.goserver/block/hash.goserver/block/model/shulker.goserver/block/model/stair.goserver/block/register.goserver/block/shulker_box.goserver/internal/nbtconv/item.goserver/item/optional_colour.goserver/player/handler.goserver/player/player.goserver/session/controllable.goserver/session/handler_player_auth_input.goserver/session/player.goserver/session/session.goserver/session/world.goserver/world/sound/block.goserver/world/viewer.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/session/world.go`:
- Around line 222-230: The position flags currently test against zero; change
them to compare previous vs new coordinates so flags are set whenever a
coordinate changed (even if the new value is 0). In the method where flags are
built (world.go, the MoveActorDelta assembly code using targetPos and
currentPos/currentPosVec), replace checks like !mgl64.FloatEqual(targetPos.X(),
0) with change-detection against the previous value (e.g.
!mgl64.FloatEqual(targetPos.X(), currentPos.X())), and do the same for Y and Z,
leaving the existing rotation-change checks (rot.Pitch() != currentRot.Pitch()
etc.) as-is; ensure you still set packet.MoveActorDeltaFlagHasX / HasY / HasZ
when differences are detected.
🪄 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: 98d73c4b-4fb8-4720-b029-4b18f8d9c42b
📒 Files selected for processing (3)
server/block/shulker_box.goserver/session/world.goserver/world/viewer.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/world/viewer.go
- server/block/shulker_box.go
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f29b359. Configure here.
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
Adds a new container block plus new player/viewer displacement APIs and a new network movement packet path, which could impact entity sync and plugin/handler implementations. Also changes NBT inventory encoding slice type, which may affect any code expecting the previous concrete type.
Overview
Implements
ShulkerBoxas a dyeable 27-slot container block (including undyed variant) that persists contents via NBT, registers all colour variants, and adds block hashing support for the newOptionalColourtype.Adds shulker lid collision/animation via a new
model.Shulkerbounding box that expands with open progress, scheduled ticks to drive open/close state, open/close sounds, and logic to push intersecting entities out of the opening lid.Extends the entity movement pipeline with forced displacement: introduces
HandleDisplaceonplayer.Handler,Displaceonsession.Controllable,Viewer.ViewEntityDelta, and aSession.ViewEntityDeltaimplementation usingMoveActorDelta(force-move), used by shulker lid pushes. Updates session container handling to recogniseContainerShulkerBox, and broadensnbtconv.InvToNBTto return[]any.Reviewed by Cursor Bugbot for commit daab568. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Release Notes