Feature/portal#19
Conversation
Portal blocks should not expose collision boxes just to trigger interdimensional travel. Route portal contact through the existing EntityInside path and let player travel state handle per-tick entry/reset. Constraint: Portal blocks are non-colliding, so BBox cannot be the travel trigger. Rejected: Scanning nearby portal blocks through Model().BBox | depends on collision geometry and duplicates EntityInsider handling. Confidence: high Scope-risk: moderate Directive: Keep portal contact detection in block EntityInside unless Dragonfly adds a broader non-collision trigger surface.
…ntal neighbour updates, while still checking vertical updates and same-axis updates so frame breaks are preserved
- Replace string-based portal scan matchers with typed PortalInterior
and Frame block interfaces (Air, Fire, Obsidian opt in); crying
obsidian no longer counts as frame and soul fire no longer
activates a portal.
- Rewrite scan as a directed rectangular validator with explicit
width/height bounds, replacing the BFS that accepted any connected
air/fire region.
- Add connectedNetherPortal flood-fill so DeactivateNetherPortal
clears every reachable portal block when a frame is broken,
avoiding orphan portal blocks left behind by a partial scan.
- Switch portal travel to a push model: Portal.EntityInside calls
TravelThroughPortal on the entity; TravelComputer.EnterPortal
records the contact and StopTravelling only resets the timer when
the entity leaves.
- Hoist activation into portal.ActivateNetherPortal and use it from
flint and steel, fire charge and Fire.Start; remove the racy
Fire.Place portal hook.
- Add cube.Axis.Faces() returning the negative and positive Face
along an Axis, and use it in the portal scan.
- Drop the Portal model BBox; EntityInside dispatch handles travel
without needing a collision shape.
- Add server/world/portal/nether_portal_test.go covering scan
acceptance and rejection (axis, frame size limits, missing side
frames, crying obsidian, soul fire) plus axis-preference,
ActivateNetherPortal and the FireCharge ignite path.
- Add server/entity/travel_test.go for TravelComputer.StopTravelling,
asserting it preserves the timer while the entity is inside a
portal and resets it once contact ends.
- Wire `go test ./...` into the pr.yml and push.yml GitHub Actions
workflows so regressions are caught alongside `make lint`.
- No remaining implementer of Place(pos cube.Pos, w *World) bool under server/.
- Flint and steel does not use that hook now. It activates portals directly in
item.FlintAndSteel.UseOnBlock via portal.ActivateNetherPortal(tx, s).
- Fire charge does the same.
- Fire spread/start also uses portal.ActivateNetherPortal directly.
- The hook is on World.setBlock, so keeping it would add a dead implicit interface check
to every block placement.
- Capture the *EntityHandle returned by RemoveEntity and pass it to AddEntity in the destination world; Tx docs warn that the original entity is unusable after removal.
- Await both source.Exec and destination.Exec so removal completes before the destination tries to add or find the entity, fixing the cross-world race.
- Bail out of travel if RemoveEntity returns nil and reset travelling.
- Restructure EnterPortal to compute travelNow under the lock and call travel() after releasing it, removing the unlock-relock dance.
- Short-circuit EnterPortal when the source world has no distinct portal destination, so non-Nether targets no-op cleanly.
- Add an instantaneous() helper that tolerates a nil Instantaneous field instead of panicking.
- Drop the dead `t.inside = false` reassignment in StopTravelling and switch the mutex to sync.Mutex (no read-locked path exists).
- Unexport Travel to travel; the entry point is EnterPortal and an external caller would skip the timed-out / awaiting-travel checks.
- Extract translatePortalPosition with godoc covering the 1:8 X/Z scaling and Y clamp to the destination's vertical range, replacing the inline transform that shifted Y by the source's Range().Min().
Portal lookup distance includes Y, so tall portals must be matched by any active interior block, not only bottom-row blocks with frame below
Portal scan helpers should return complete validated portal interiors or no positions at all.
|
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements a complete Nether portal system with portal block types, interdimensional entity traversal infrastructure, frame detection and activation logic, behavior integration across all entity types, item-based triggering, and comprehensive test coverage including entity traversal and portal lifecycle scenarios. ChangesNether Portal System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 4
🧹 Nitpick comments (3)
server/entity/travel.go (1)
105-113: 💤 Low valueExtract the portal search radius into a named constant.
128is an unexplained magic number and the same value will likely be needed elsewhere (Find/Create call sites, configuration, tests). A package-levelconstmakes the intent explicit and easy to tune.♻️ Proposed refactor
+// netherPortalSearchRadius is the radius, in blocks, used when looking for an +// existing destination portal before creating a new one. +const netherPortalSearchRadius = 128 + ... - if netherPortal, ok := portal.FindOrCreateNetherPortal(tx, pos, 128); ok { + if netherPortal, ok := portal.FindOrCreateNetherPortal(tx, pos, netherPortalSearchRadius); ok { spawn = netherPortal.Spawn().Vec3Middle() }🤖 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/entity/travel.go` around lines 105 - 113, Replace the magic literal 128 with a package-level constant to make the portal search radius explicit: add a const (e.g. NETHER_PORTAL_SEARCH_RADIUS) at the top of server/entity/travel.go and use it in the call to portal.FindOrCreateNetherPortal(tx, pos, NETHER_PORTAL_SEARCH_RADIUS); ensure other code references (the same call site inside the anonymous func that sets spawn and later Teleport on the Traveller returned by tx.AddEntity(handle)) use that constant too so the value is centralized for tests/configuration.server/world/portal/nether.go (1)
281-312: ⚡ Quick winHoist
portal(n.axis)out of these loops.Both
Activate()andActivated()callportal(n.axis)once per position; today that's aworld.BlockByNamecall (with a map allocation for the axis arg) per portal block, every check. Even after the suggested caching inscan.go, hoisting the resolved block into a local makes the intent obvious and avoids the per-iteration map lookup.♻️ Proposed refactor
func (n Nether) Activate() { + b := portal(n.axis) for _, pos := range n.Positions() { - n.tx.SetBlock(pos, portal(n.axis), nil) + n.tx.SetBlock(pos, b, nil) } } ... func (n Nether) Activated() bool { + b := portal(n.axis) for _, pos := range n.Positions() { - if n.tx.Block(pos) != portal(n.axis) { + if n.tx.Block(pos) != b { return false } } return 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 `@server/world/portal/nether.go` around lines 281 - 312, The loops in Activate() and Activated() call portal(n.axis) for every position; hoist that call into a local variable to avoid repeated world.BlockByName/map allocations: in Activate(), compute b := portal(n.axis) before the for _, pos := range n.Positions() loop and call n.tx.SetBlock(pos, b, nil); in Activated(), compute b := portal(n.axis) once and compare n.tx.Block(pos) != b inside the loop. Keep deactivate(tx, n.Positions()) unchanged.server/world/portal/scan.go (1)
166-191: ⚡ Quick winCache
air/portal/obsidianinstead of resolving by name on every call.These helpers invoke
world.BlockByName(which does a map lookup plus ahashPropertiescall, and a map allocation for theportal_axisarg inportal()) on every invocation. They're hit in hot paths innether.go—CreateNetherPortaldoestx.Block(p) != air()inside triple-nested loops, andNether.Activated()/Activate()callportal(n.axis)once per portal block.BlockByNamereturns canonical instances from theblocksarray (indexed by runtime ID), so caching at init time is safe for comparison semantics and provides a single deterministic place for registration panics.♻️ Proposed refactor
-// air returns an air block. -func air() world.Block { - a, ok := world.BlockByName("minecraft:air", nil) - if !ok { - panic("could not find air block") - } - return a -} - -// portal returns a portal block. -func portal(axis cube.Axis) world.Block { - p, ok := world.BlockByName("minecraft:portal", map[string]any{"portal_axis": axis.String()}) - if !ok { - panic("could not find portal block") - } - return p -} - -// obsidian returns an obsidian block. -func obsidian() world.Block { - o, ok := world.BlockByName("minecraft:obsidian", nil) - if !ok { - panic("could not find obsidian block") - } - return o -} +var ( + airBlock world.Block + obsidianBlock world.Block + portalBlocks = map[cube.Axis]world.Block{} +) + +func init() { + var ok bool + if airBlock, ok = world.BlockByName("minecraft:air", nil); !ok { + panic("could not find air block") + } + if obsidianBlock, ok = world.BlockByName("minecraft:obsidian", nil); !ok { + panic("could not find obsidian block") + } + for _, axis := range []cube.Axis{cube.X, cube.Z} { + p, ok := world.BlockByName("minecraft:portal", map[string]any{"portal_axis": axis.String()}) + if !ok { + panic("could not find portal block") + } + portalBlocks[axis] = p + } +} + +func air() world.Block { return airBlock } +func obsidian() world.Block { return obsidianBlock } +func portal(axis cube.Axis) world.Block { return portalBlocks[axis] }🤖 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/world/portal/scan.go` around lines 166 - 191, The helpers air(), portal(axis), and obsidian() repeatedly call world.BlockByName on hot paths; change them to resolve and cache their block values at package init so subsequent calls just return the cached values (for portal precompute both axis-aligned portal blocks or cache a map from cube.Axis to world.Block). Move the lookup and panic into an init() (or package-level var initialization) using the same world.BlockByName calls and have air(), obsidian(), and portal(axis) return the cached variables (or map lookup) to avoid repeated map allocations and hashProperties work.
🤖 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/entity/travel.go`:
- Around line 91-118: In the goroutine launched by travel(), the branch where
handle == nil only clears t.travelling but leaves timer state set; update that
failure branch (after RemoveEntity returns nil) to acquire t.mu, set both
t.travelling = false and clear the timer/await flags (e.g., t.timedOut = false
and t.awaitingTravel = false or the equivalent fields your code uses), then
release the lock; this ensures subsequent calls (and StopTravelling) can retry
when RemoveEntity fails. Also keep the early return behavior and use the same
locking pattern as elsewhere.
- Around line 91-118: The goroutine can block indefinitely on <-source.Exec(...)
or <-destination.Exec(...) if a world shuts down because handleTransactions
closes without draining queue; change the logic to use a cancellable/abortable
Exec (or add a context/closing-channel-aware Exec variant) so the travel
goroutine can select between the Exec result and the world's shutdown signal:
replace the direct <-source.Exec(...) and <-destination.Exec(...) calls in the
goroutine with a select that waits for either the Exec result or the world's
queueClosing/closing channel (or a context.Done), and if the Exec is
aborted/closed return the entity handle to a safe state (re-add it to source via
source.Exec or drop it with appropriate tx call) and set t.travelling = false;
alternatively modify handleTransactions to drain or close pending transaction
result channels on queueClosing so existing callers of
source.Exec/destination.Exec never block—ensure you reference source.Exec,
destination.Exec, handleTransactions, queueClosing, Traveller.Teleport and
world.Tx in the change so the goroutine always unblocks on world shutdown.
In `@server/player/conf.go`:
- Line 72: The Instantaneous closure passed to entity.TravelComputer currently
captures cfg.GameMode at construction time; change it so the closure reads the
live player game-mode field (pdata.gameMode or p.gameMode) instead of
cfg.GameMode so SetGameMode updates take effect — locate the TravelComputer
construction (tc: &entity.TravelComputer{Instantaneous: ...}) and replace the
capture of cfg.GameMode with a zero-arg function that returns whether the
current pdata.gameMode == world.GameModeCreative.
In `@server/world/portal/nether.go`:
- Around line 95-121: FindNetherPortal is doing redundant work: hoist the
dimension range retrieval out of the nested x/z loops by assigning r :=
tx.World().Dimension().Range() once before iterating, and use that r for the y
loop; change the x and z loop bounds to be inclusive (for x := pos.X()-radius; x
<= pos.X()+radius; x++ and same for z) so the search is symmetric; keep the
inner y loop using r.Max()/r.Min() and the existing checks (tx.Block,
portalBlock, NetherPortalFromPos) unchanged. Consider later replacing the
brute-force column scan with a chunk/POI-based filter before rollout to scale.
---
Nitpick comments:
In `@server/entity/travel.go`:
- Around line 105-113: Replace the magic literal 128 with a package-level
constant to make the portal search radius explicit: add a const (e.g.
NETHER_PORTAL_SEARCH_RADIUS) at the top of server/entity/travel.go and use it in
the call to portal.FindOrCreateNetherPortal(tx, pos,
NETHER_PORTAL_SEARCH_RADIUS); ensure other code references (the same call site
inside the anonymous func that sets spawn and later Teleport on the Traveller
returned by tx.AddEntity(handle)) use that constant too so the value is
centralized for tests/configuration.
In `@server/world/portal/nether.go`:
- Around line 281-312: The loops in Activate() and Activated() call
portal(n.axis) for every position; hoist that call into a local variable to
avoid repeated world.BlockByName/map allocations: in Activate(), compute b :=
portal(n.axis) before the for _, pos := range n.Positions() loop and call
n.tx.SetBlock(pos, b, nil); in Activated(), compute b := portal(n.axis) once and
compare n.tx.Block(pos) != b inside the loop. Keep deactivate(tx, n.Positions())
unchanged.
In `@server/world/portal/scan.go`:
- Around line 166-191: The helpers air(), portal(axis), and obsidian()
repeatedly call world.BlockByName on hot paths; change them to resolve and cache
their block values at package init so subsequent calls just return the cached
values (for portal precompute both axis-aligned portal blocks or cache a map
from cube.Axis to world.Block). Move the lookup and panic into an init() (or
package-level var initialization) using the same world.BlockByName calls and
have air(), obsidian(), and portal(axis) return the cached variables (or map
lookup) to avoid repeated map allocations and hashProperties work.
🪄 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: ab67979a-d8b0-46cc-924c-ad3904a93126
📒 Files selected for processing (21)
.github/workflows/pr.yml.github/workflows/push.ymlserver/block/air.goserver/block/cube/axis.goserver/block/fire.goserver/block/hash.goserver/block/model/portal.goserver/block/obsidian.goserver/block/portal.goserver/block/register.goserver/entity/ender_pearl.goserver/entity/projectile.goserver/entity/travel.goserver/entity/travel_test.goserver/item/fire_charge.goserver/item/flint_and_steel.goserver/player/conf.goserver/player/player.goserver/world/portal/nether.goserver/world/portal/nether_test.goserver/world/portal/scan.go
| go func() { | ||
| spawn := pos.Vec3Middle() | ||
|
|
||
| var handle *world.EntityHandle | ||
| <-source.Exec(func(tx *world.Tx) { | ||
| handle = tx.RemoveEntity(e) | ||
| }) | ||
| if handle == nil { | ||
| t.mu.Lock() | ||
| t.travelling = false | ||
| t.mu.Unlock() | ||
| return | ||
| } | ||
|
|
||
| <-destination.Exec(func(tx *world.Tx) { | ||
| if netherPortal, ok := portal.FindOrCreateNetherPortal(tx, pos, 128); ok { | ||
| spawn = netherPortal.Spawn().Vec3Middle() | ||
| } | ||
|
|
||
| if ent, ok := tx.AddEntity(handle).(Traveller); ok { | ||
| ent.Teleport(spawn) | ||
| } | ||
| }) | ||
|
|
||
| t.mu.Lock() | ||
| defer t.mu.Unlock() | ||
| t.travelling = false | ||
| }() |
There was a problem hiding this comment.
timedOut is left set if RemoveEntity returns nil.
When handle == nil (entity already removed elsewhere, world swap in flight, etc.) the goroutine clears travelling but not timedOut/awaitingTravel. Because travel() set timedOut=true synchronously at line 89 before launching this goroutine, the entity won't be allowed to portal again until StopTravelling observes a tick where inside == false — which may never happen if it's still standing on the portal block and the source-side traversal silently failed. Consider clearing the timer state on this failure branch so a subsequent attempt can proceed.
🛡️ Proposed fix
if handle == nil {
t.mu.Lock()
- t.travelling = false
+ t.travelling, t.timedOut, t.awaitingTravel = false, false, false
t.mu.Unlock()
return
}🤖 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/entity/travel.go` around lines 91 - 118, In the goroutine launched by
travel(), the branch where handle == nil only clears t.travelling but leaves
timer state set; update that failure branch (after RemoveEntity returns nil) to
acquire t.mu, set both t.travelling = false and clear the timer/await flags
(e.g., t.timedOut = false and t.awaitingTravel = false or the equivalent fields
your code uses), then release the lock; this ensures subsequent calls (and
StopTravelling) can retry when RemoveEntity fails. Also keep the early return
behavior and use the same locking pattern as elsewhere.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm world.World.Exec semantics around shutdown / closed state.
ast-grep --pattern $'func ($_ *World) Exec($$$) $_ {
$$$
}'
rg -nP --type=go -C5 '\bfunc \(\s*\w+\s+\*World\s*\)\s+Exec\b'
rg -nP --type=go -C3 '\bclose\(.*queue.*\)|world is closed'Repository: HashimTheArab/dragonfly
Length of output: 1291
🏁 Script executed:
# Examine world shutdown logic and queue handling
rg -n "w.queue|queueClosing" server/world/world.go -A 2 -B 2 | head -80
# Look for the queue field initialization and processing
rg -n "queue.*make\|normalTransaction" server/world/world.go -B 2 -A 2
# Find the goroutine that processes w.queue
rg -n "for.*w.queue|select.*w.queue" server/world/world.go -B 3 -A 10Repository: HashimTheArab/dragonfly
Length of output: 827
🏁 Script executed:
# Check queue initialization and buffering
rg -n "queue.*chan|make.*queue" server/world/world.go -B 2 -A 2
# Check if there's any closed queue detection in Exec
rg -n "Exec" server/world/world.go -A 5 | grep -A 5 "func.*Exec"
# Look for any panic recovery or protection in Exec callers
rg -n "recover|closed" server/world/world.go | head -20Repository: HashimTheArab/dragonfly
Length of output: 866
🏁 Script executed:
# Look for queue initialization/creation
rg -n "w\.queue.*=" server/world/world.go | head -20
# Search for New function that creates World
rg -n "func New\|queue:" server/world/world.go | head -30Repository: HashimTheArab/dragonfly
Length of output: 49
🏁 Script executed:
# Find World constructor/New function
rg -n "func New.*World|func.*World.*{" server/world/world.go | head -5
# Get the constructor to see queue initialization
sed -n '900,1000p' server/world/world.go | head -100
# Or search backwards from where queue is used
head -200 server/world/world.go | tail -100Repository: HashimTheArab/dragonfly
Length of output: 6399
🏁 Script executed:
# Search for New function creating World
rg -n "^func New" server/world/world.go
# Search for queue field assignment in constructor
rg -n "queue.*make\|queue.*=" server/world/world.go
# Get file structure - look at beginning and find constructor
head -150 server/world/world.goRepository: HashimTheArab/dragonfly
Length of output: 4570
🏁 Script executed:
# Find Config.New() method
rg -n "func.*Config.*New|func \(c Config\)" server/world/world.go -A 30
# Or search in world package for Config
rg -n "type Config struct" server/world/ -A 20Repository: HashimTheArab/dragonfly
Length of output: 3089
🏁 Script executed:
# Find Config.New() implementation
rg -n "func \(c Config\) New\(\)" server/world/ -A 40Repository: HashimTheArab/dragonfly
Length of output: 49
🏁 Script executed:
# Search for New method on Config struct
rg -n "func.*New.*World" server/world/conf.go -A 30
# Or search for queue initialization more broadly
rg -n "make.*chan\|queue" server/world/conf.goRepository: HashimTheArab/dragonfly
Length of output: 949
🏁 Script executed:
# Get more of the New() constructor
sed -n '67,150p' server/world/conf.goRepository: HashimTheArab/dragonfly
Length of output: 1462
🏁 Script executed:
# Now look at shutdown sequence around line 1074
sed -n '1050,1090p' server/world/world.goRepository: HashimTheArab/dragonfly
Length of output: 1027
🏁 Script executed:
# Look at the travel.go code around the portal transition
sed -n '85,125p' server/entity/travel.goRepository: HashimTheArab/dragonfly
Length of output: 1286
🏁 Script executed:
# Check if queue is ever explicitly drained before shutdown
rg -n "drain\|flush\|queue" server/world/world.go | grep -i "drain\|flush\|close.*queue"
# Check if there's any protection against sending on closed queue
rg -n "panic\|recover\|closed" server/world/world.go -B 2 -A 2 | head -40Repository: HashimTheArab/dragonfly
Length of output: 49
🏁 Script executed:
# Confirm w.queue is never explicitly closed
rg -n "close.*queue|queue.*close" server/world/world.go
# Check what happens if we send to closed channel
rg -n "w.queue <-" server/world/world.go
# Verify the final state: is queueClosing the only thing closed?
rg -n "close\(" server/world/world.goRepository: HashimTheArab/dragonfly
Length of output: 297
Verify the goroutine cannot deadlock on world shutdown.
The traversal goroutine blocks on <-source.Exec(...) and <-destination.Exec(...). If either world shuts down between the entity entering the portal and the goroutine executing, the Exec transactions may be abandoned in the queue without their returned channels ever being closed. The handleTransactions loop exits immediately upon receiving queueClosing without draining pending transactions, causing the goroutine to block indefinitely on a channel that will never close.
🤖 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/entity/travel.go` around lines 91 - 118, The goroutine can block
indefinitely on <-source.Exec(...) or <-destination.Exec(...) if a world shuts
down because handleTransactions closes without draining queue; change the logic
to use a cancellable/abortable Exec (or add a context/closing-channel-aware Exec
variant) so the travel goroutine can select between the Exec result and the
world's shutdown signal: replace the direct <-source.Exec(...) and
<-destination.Exec(...) calls in the goroutine with a select that waits for
either the Exec result or the world's queueClosing/closing channel (or a
context.Done), and if the Exec is aborted/closed return the entity handle to a
safe state (re-add it to source via source.Exec or drop it with appropriate tx
call) and set t.travelling = false; alternatively modify handleTransactions to
drain or close pending transaction result channels on queueClosing so existing
callers of source.Exec/destination.Exec never block—ensure you reference
source.Exec, destination.Exec, handleTransactions, queueClosing,
Traveller.Teleport and world.Tx in the change so the goroutine always unblocks
on world shutdown.
| func FindNetherPortal(tx *world.Tx, pos cube.Pos, radius int) (Nether, bool) { | ||
| if tx.World().Dimension() == world.End { | ||
| return Nether{}, false | ||
| } | ||
|
|
||
| closest, closestDist, found := Nether{}, math.MaxFloat64, false | ||
| for x := pos.X() - radius; x < pos.X()+radius; x++ { | ||
| for z := pos.Z() - radius; z < pos.Z()+radius; z++ { | ||
| r := tx.World().Dimension().Range() | ||
| for y := r.Max(); y >= r.Min(); y-- { | ||
| selectedPos := cube.Pos{x, y, z} | ||
| if p, ok := tx.Block(selectedPos).(portalBlock); ok && p.Portal() == world.Nether { | ||
| if n, ok := NetherPortalFromPos(tx, selectedPos); ok && n.Framed() && n.Activated() { | ||
| dist := selectedPos.Vec3().Sub(pos.Vec3()).Len() | ||
| if dist < closestDist { | ||
| closestDist, closest, found = dist, n, true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| if !found { | ||
| return Nether{}, false | ||
| } | ||
| return closest, true | ||
| } |
There was a problem hiding this comment.
FindNetherPortal scans far too many blocks per call.
With the default radius of 128 from travel.go, this iterates (2·128)² · height positions — roughly 25M tx.Block lookups on a standard Overworld height range, per portal traversal. On a busy server this will stall transactions noticeably.
A few options, in increasing order of effort:
- Hoist
tx.World().Dimension().Range()out of thex/zloops; it's recomputed ~65k times today. - Make the bounds inclusive on both ends (
x <= pos.X()+radius) so the search isn't asymmetric aroundpos. - Replace the brute-force scan with a chunk-level filter (only iterate columns in chunks that actually contain any portal blocks) — vanilla uses a "PoiManager" precisely to avoid this.
At minimum, please apply (1) and revisit (3) before this is exercised at scale.
♻️ Minimum-fix diff (hoist + inclusive bounds)
func FindNetherPortal(tx *world.Tx, pos cube.Pos, radius int) (Nether, bool) {
if tx.World().Dimension().Range() == (cube.Range{}) {
}
if tx.World().Dimension() == world.End {
return Nether{}, false
}
closest, closestDist, found := Nether{}, math.MaxFloat64, false
- for x := pos.X() - radius; x < pos.X()+radius; x++ {
- for z := pos.Z() - radius; z < pos.Z()+radius; z++ {
- r := tx.World().Dimension().Range()
- for y := r.Max(); y >= r.Min(); y-- {
+ r := tx.World().Dimension().Range()
+ for x := pos.X() - radius; x <= pos.X()+radius; x++ {
+ for z := pos.Z() - radius; z <= pos.Z()+radius; z++ {
+ for y := r.Max(); y >= r.Min(); y-- {
selectedPos := cube.Pos{x, y, z}
if p, ok := tx.Block(selectedPos).(portalBlock); ok && p.Portal() == world.Nether {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func FindNetherPortal(tx *world.Tx, pos cube.Pos, radius int) (Nether, bool) { | |
| if tx.World().Dimension() == world.End { | |
| return Nether{}, false | |
| } | |
| closest, closestDist, found := Nether{}, math.MaxFloat64, false | |
| for x := pos.X() - radius; x < pos.X()+radius; x++ { | |
| for z := pos.Z() - radius; z < pos.Z()+radius; z++ { | |
| r := tx.World().Dimension().Range() | |
| for y := r.Max(); y >= r.Min(); y-- { | |
| selectedPos := cube.Pos{x, y, z} | |
| if p, ok := tx.Block(selectedPos).(portalBlock); ok && p.Portal() == world.Nether { | |
| if n, ok := NetherPortalFromPos(tx, selectedPos); ok && n.Framed() && n.Activated() { | |
| dist := selectedPos.Vec3().Sub(pos.Vec3()).Len() | |
| if dist < closestDist { | |
| closestDist, closest, found = dist, n, true | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| if !found { | |
| return Nether{}, false | |
| } | |
| return closest, true | |
| } | |
| func FindNetherPortal(tx *world.Tx, pos cube.Pos, radius int) (Nether, bool) { | |
| if tx.World().Dimension() == world.End { | |
| return Nether{}, false | |
| } | |
| closest, closestDist, found := Nether{}, math.MaxFloat64, false | |
| r := tx.World().Dimension().Range() | |
| for x := pos.X() - radius; x <= pos.X()+radius; x++ { | |
| for z := pos.Z() - radius; z <= pos.Z()+radius; z++ { | |
| for y := r.Max(); y >= r.Min(); y-- { | |
| selectedPos := cube.Pos{x, y, z} | |
| if p, ok := tx.Block(selectedPos).(portalBlock); ok && p.Portal() == world.Nether { | |
| if n, ok := NetherPortalFromPos(tx, selectedPos); ok && n.Framed() && n.Activated() { | |
| dist := selectedPos.Vec3().Sub(pos.Vec3()).Len() | |
| if dist < closestDist { | |
| closestDist, closest, found = dist, n, true | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| if !found { | |
| return Nether{}, false | |
| } | |
| return closest, 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 `@server/world/portal/nether.go` around lines 95 - 121, FindNetherPortal is
doing redundant work: hoist the dimension range retrieval out of the nested x/z
loops by assigning r := tx.World().Dimension().Range() once before iterating,
and use that r for the y loop; change the x and z loop bounds to be inclusive
(for x := pos.X()-radius; x <= pos.X()+radius; x++ and same for z) so the search
is symmetric; keep the inner y loop using r.Max()/r.Min() and the existing
checks (tx.Block, portalBlock, NetherPortalFromPos) unchanged. Consider later
replacing the brute-force column scan with a chunk/POI-based filter before
rollout to scale.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
FindNetherPortal brute-force scans millions of blocks
Medium Severity
FindNetherPortal iterates every block in a 256×256×(world height) volume when called with radius=128. In the Nether (128 Y levels) that's ~8.4 million block reads; in the Overworld (384 Y levels) it's ~25 million. This runs inside a world transaction triggered by any entity portal travel, likely causing noticeable server stalls.
Reviewed by Cursor Bugbot for commit da4ed2c. Configure here.
HashimTheArab
force-pushed
the
feature/portal
branch
from
da4ed2c to
982e1b3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
server/entity/travel.go (2)
142-152:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
timedOutandawaitingTravelare still left set whenRemoveEntityreturns nil — now in both code paths.The refactor split
travel()andtravelQueued(), but both failure branches only cleartravellingand leavetimedOut=true(set at lines 143 and 184 respectively). After this,enterPortal()will reject the entity at line 76 untilStopPortalContact()observes a tick whereinside == false, which may never happen if the entity is still standing on the portal block. A subsequent retry can therefore never proceed.🛡️ Proposed fix (apply in both
travel()andtravelQueued())if handle == nil { t.mu.Lock() - t.travelling = false + t.travelling, t.timedOut, t.awaitingTravel = false, false, false t.mu.Unlock() return }Also applies to: 183-197
🤖 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/entity/travel.go` around lines 142 - 152, When tx.RemoveEntity(e) returns nil the code only clears t.travelling but leaves t.timedOut and t.awaitingTravel set, which prevents enterPortal() from allowing retries; update both travel() and travelQueued() failure branches (the blocks after calling tx.RemoveEntity(e)) to reset all three flags—t.travelling = false, t.timedOut = false, and t.awaitingTravel = false—under the same mutex used (t.mu) so StopPortalContact() / enterPortal() can observe inside==false and allow subsequent retries.
154-169:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftGoroutine can still deadlock on world shutdown — issue now exists on both
source.Execanddestination.Execpaths.Each goroutine blocks on
<-source.Exec(...)(line 189) and/or<-destination.Exec(...)(lines 155, 199). If the corresponding world is shut down between portal entry and the transaction firing,handleTransactionsexits onqueueClosingwithout draining or closing the result channels of pending transactions, so the goroutine blocks indefinitely. IntravelQueued()this is even worse: if the destination world shuts down aftersource.Execremoved the handle but beforedestination.Execruns, the entity is removed from source, the handle is captured by the leaked goroutine, and the entity effectively disappears.Either:
- make the goroutine select between the
Execchannel and a world-closing/context.Done()signal, and on shutdown re-add the handle to the source (or otherwise hand it to a safe owner) before returning, or- have
handleTransactionsclose pending transaction result channels whenqueueClosingfires so existing callers always unblock.#!/bin/bash # Confirm queueClosing handling still leaks pending transactions and that no fix has landed since the past review. rg -nP --type=go -C5 'queueClosing|handleTransactions\b' server/world/ rg -nP --type=go -C3 '\bfunc\s+\(\s*\w+\s+\*World\s*\)\s+Exec\b' server/world/ # Verify no shutdown-aware Exec variant exists yet. rg -nP --type=go 'ExecContext|ExecCtx|TryExec' server/world/Also applies to: 187-213
🤖 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/entity/travel.go` around lines 154 - 169, The goroutine blocking on destination.Exec (and similarly on source.Exec in travelQueued) can deadlock if the world shuts down; update the logic so the Exec call is cancellable: modify the Exec invocation in travel.go (the anonymous goroutine that calls destination.Exec / source.Exec and then t.finishTravel) to select between the Exec result channel and a world-shutdown signal (use the world’s queueClosing/context.Done equivalent exposed by the World API), and on shutdown ensure you return the entity handle to a safe owner (re-add the handle to the source world or call the existing cleanup path) before unlocking t.mu and setting t.travelling=false; alternatively, if you prefer centralizing fixes, update handleTransactions to close all pending transaction result channels when queueClosing fires so callers of destination.Exec/source.Exec always unblock (make sure to reference handleTransactions and queueClosing in that change).
🧹 Nitpick comments (3)
server/entity/ent.go (1)
131-156: 💤 Low valueConsider a short comment explaining the portal-travel sequence in Tick.
The flow —
deferPortalTravelflag, twofinishPendingPortalTravelcalls separated bym.SendandcheckPortalInsiders, and skipping the age increment on terminal travel — is non-obvious without readingent_portal.goandtravel.goside-by-side. A few inline comments documenting (a) why behaviour-queued travel is finished before the movement is sent, (b) why a second pass is needed aftercheckPortalInsiders, and (c) why age must not advance on terminal travel will save future readers a round trip.📝 Suggested comments
m := e.Behaviour().Tick(e, tx) + // If the behaviour itself queued portal travel (e.g. a projectile hit a + // portal), finish it before broadcasting the movement: a successful + // terminal travel removes the entity from this world. if e.finishPendingPortalTravel(tx) { return } if m != nil { m.Send() } + // After the move, recheck for portal-block contact and finish any + // terminal travel queued during this tick. Age is intentionally not + // advanced when terminal travel succeeds — the entity now belongs to + // the destination world. if e.checkPortalInsiders() && e.finishPendingPortalTravel(tx) { return } e.stopPortalContact()🤖 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/entity/ent.go` around lines 131 - 156, Add brief inline comments inside Ent.Tick explaining the portal-travel sequence: document why deferPortalTravel is set for the duration of Tick, why finishPendingPortalTravel is called immediately after Behaviour().Tick (to complete any behaviour-queued/terminal travel before sending movement), why a second finishPendingPortalTravel call is required after checkPortalInsiders (to handle arrivals triggered by insider checks that occur after movement was sent), and why the age increment (e.data.Age += ...) is skipped when a terminal travel completes; place these comments adjacent to the relevant symbols (Tick, deferPortalTravel, Behaviour().Tick, m.Send, finishPendingPortalTravel, checkPortalInsiders, stopPortalContact and the age increment) so readers can understand the control flow without inspecting ent_portal.go or travel.go.server/entity/projectile.go (1)
110-121: ⚡ Quick winField name collision with embedded
BaseBehaviour.portalTravel.
ProjectileBehaviourdeclaresportalTravel boolat line 120 while the embeddedBaseBehaviouralso declaresportalTravel *PortalTravelComputer. The outer bool shadows the inner pointer for unqualified access, so the two fields are only distinguishable vialt.BaseBehaviour.portalTravel(or the inheritedPortalTravelComputer()accessor). The compiler accepts it, but the semantics ("currently in portal contact" vs. "has already travelled at some point") are very different and a future reader/refactor can easily touch the wrong one.Renaming the bool to something like
travelledortravelledThroughPortalremoves the ambiguity and reads better at the use sites.♻️ Proposed rename
@@ ProjectileBehaviour struct collisionPos cube.Pos collided bool - portalTravel bool + travelled bool @@ HandlePortalTravel -func (lt *ProjectileBehaviour) HandlePortalTravel(world.Dimension, world.Dimension) { - lt.portalTravel = true -} +func (lt *ProjectileBehaviour) HandlePortalTravel(world.Dimension, world.Dimension) { + lt.travelled = true +} @@ PortalTravel -func (lt *ProjectileBehaviour) PortalTravel() bool { - return lt.portalTravel -} +func (lt *ProjectileBehaviour) PortalTravel() bool { + return lt.travelled +}🤖 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/entity/projectile.go` around lines 110 - 121, ProjectileBehaviour currently declares a bool field portalTravel that shadows the embedded BaseBehaviour.portalTravel (*PortalTravelComputer), which is confusing and error-prone; rename the bool (for example to travelledThroughPortal or travelled) in the ProjectileBehaviour struct and update all usages in methods referencing ProjectileBehaviour.portalTravel to use the new name, leaving BaseBehaviour.portalTravel (or its PortalTravelComputer() accessor) untouched so there is no shadowing or ambiguity between the boolean "has travelled" flag and the embedded portal travel computer.server/entity/travel.go (1)
132-214: ⚡ Quick winDeduplicate
travel()andtravelQueued().The two functions share identical position translation, identical flag setup, identical destination-side
Execblock (lines 155-164 vs 199-208), and identical finaltravelling=falsecleanup. They differ only in how the source-sideRemoveEntityis performed (in-line vs viasource.Exec). Extracting the shared destination logic into a helper (and the failure-path cleanup into another) would prevent the two paths from drifting — which is already happening for thetimedOutfix above, since any future correction has to be applied twice.♻️ Sketch of a shared destination/cleanup helper
// reset clears travelling flags; if full is true, also clears timedOut/awaitingTravel // so a failed transfer does not strand the entity in the timed-out state. func (t *PortalTravelComputer) reset(full bool) { t.mu.Lock() defer t.mu.Unlock() t.travelling = false if full { t.timedOut, t.awaitingTravel = false, false } } // placeAtDestination moves handle into destination at a portal-derived spawn. func (t *PortalTravelComputer) placeAtDestination(handle *world.EntityHandle, pos cube.Pos, destination *world.World, sourceDim, destinationDim world.Dimension) { <-destination.Exec(func(tx *world.Tx) { spawn := pos.Vec3Middle() if netherPortal, ok := portal.FindOrCreateNetherPortal(tx, pos, 128); ok { spawn = netherPortal.Spawn().Vec3Middle() } if e, ok := tx.AddEntityAt(handle, spawn).(Traveller); ok { t.finishTravel(e, spawn, sourceDim, destinationDim) } }) t.reset(false) }
travel()andtravelQueued()then reduce to setting uphandle/posand invokingplaceAtDestinationfrom a goroutine.🤖 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/entity/travel.go` around lines 132 - 214, The travel() and travelQueued() methods duplicate position translation, flag setup, destination Exec logic and cleanup; extract the shared destination logic and reset cleanup into helpers on PortalTravelComputer (e.g. placeAtDestination(handle *world.EntityHandle, pos cube.Pos, destination *world.World, sourceDim, destinationDim world.Dimension) that runs the destination.Exec block and calls finishTravel, and reset(full bool) to clear travelling/timedOut/awaitingTravel appropriately), then have travel() and travelQueued() only obtain the handle (inline RemoveEntity or via source.Exec), set flags, and launch a goroutine that calls placeAtDestination; ensure you reference translatePortalPosition, portal.FindOrCreateNetherPortal and tx.AddEntityAt parts moved intact and that failure paths call reset(true) so timedOut/awaitingTravel aren’t left set.
🤖 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.
Duplicate comments:
In `@server/entity/travel.go`:
- Around line 142-152: When tx.RemoveEntity(e) returns nil the code only clears
t.travelling but leaves t.timedOut and t.awaitingTravel set, which prevents
enterPortal() from allowing retries; update both travel() and travelQueued()
failure branches (the blocks after calling tx.RemoveEntity(e)) to reset all
three flags—t.travelling = false, t.timedOut = false, and t.awaitingTravel =
false—under the same mutex used (t.mu) so StopPortalContact() / enterPortal()
can observe inside==false and allow subsequent retries.
- Around line 154-169: The goroutine blocking on destination.Exec (and similarly
on source.Exec in travelQueued) can deadlock if the world shuts down; update the
logic so the Exec call is cancellable: modify the Exec invocation in travel.go
(the anonymous goroutine that calls destination.Exec / source.Exec and then
t.finishTravel) to select between the Exec result channel and a world-shutdown
signal (use the world’s queueClosing/context.Done equivalent exposed by the
World API), and on shutdown ensure you return the entity handle to a safe owner
(re-add the handle to the source world or call the existing cleanup path) before
unlocking t.mu and setting t.travelling=false; alternatively, if you prefer
centralizing fixes, update handleTransactions to close all pending transaction
result channels when queueClosing fires so callers of
destination.Exec/source.Exec always unblock (make sure to reference
handleTransactions and queueClosing in that change).
---
Nitpick comments:
In `@server/entity/ent.go`:
- Around line 131-156: Add brief inline comments inside Ent.Tick explaining the
portal-travel sequence: document why deferPortalTravel is set for the duration
of Tick, why finishPendingPortalTravel is called immediately after
Behaviour().Tick (to complete any behaviour-queued/terminal travel before
sending movement), why a second finishPendingPortalTravel call is required after
checkPortalInsiders (to handle arrivals triggered by insider checks that occur
after movement was sent), and why the age increment (e.data.Age += ...) is
skipped when a terminal travel completes; place these comments adjacent to the
relevant symbols (Tick, deferPortalTravel, Behaviour().Tick, m.Send,
finishPendingPortalTravel, checkPortalInsiders, stopPortalContact and the age
increment) so readers can understand the control flow without inspecting
ent_portal.go or travel.go.
In `@server/entity/projectile.go`:
- Around line 110-121: ProjectileBehaviour currently declares a bool field
portalTravel that shadows the embedded BaseBehaviour.portalTravel
(*PortalTravelComputer), which is confusing and error-prone; rename the bool
(for example to travelledThroughPortal or travelled) in the ProjectileBehaviour
struct and update all usages in methods referencing
ProjectileBehaviour.portalTravel to use the new name, leaving
BaseBehaviour.portalTravel (or its PortalTravelComputer() accessor) untouched so
there is no shadowing or ambiguity between the boolean "has travelled" flag and
the embedded portal travel computer.
In `@server/entity/travel.go`:
- Around line 132-214: The travel() and travelQueued() methods duplicate
position translation, flag setup, destination Exec logic and cleanup; extract
the shared destination logic and reset cleanup into helpers on
PortalTravelComputer (e.g. placeAtDestination(handle *world.EntityHandle, pos
cube.Pos, destination *world.World, sourceDim, destinationDim world.Dimension)
that runs the destination.Exec block and calls finishTravel, and reset(full
bool) to clear travelling/timedOut/awaitingTravel appropriately), then have
travel() and travelQueued() only obtain the handle (inline RemoveEntity or via
source.Exec), set flags, and launch a goroutine that calls placeAtDestination;
ensure you reference translatePortalPosition, portal.FindOrCreateNetherPortal
and tx.AddEntityAt parts moved intact and that failure paths call reset(true) so
timedOut/awaitingTravel aren’t left set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 749e79fd-5bb3-4739-a5e6-150b8b7bc905
📒 Files selected for processing (23)
server/block/cube/face.goserver/block/cube/pos.goserver/block/portal.goserver/entity/area_effect_cloud_behaviour.goserver/entity/base_behaviour.goserver/entity/ent.goserver/entity/ent_portal.goserver/entity/experience_orb_behaviour.goserver/entity/falling_block_behaviour.goserver/entity/firework_behaviour.goserver/entity/item_behaviour.goserver/entity/passive.goserver/entity/projectile.goserver/entity/stationary.goserver/entity/travel.goserver/entity/travel_test.goserver/player/conf.goserver/player/player.goserver/world/entity.goserver/world/handler.goserver/world/portal/nether_test.goserver/world/tx.goserver/world/world.go
✅ Files skipped from review due to trivial changes (1)
- server/world/handler.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/player/conf.go
- server/block/portal.go
- server/world/portal/nether_test.go
| // Bounds ... | ||
| func (n Nether) Bounds() (int, int) { | ||
| return n.w, n.h | ||
| } |
There was a problem hiding this comment.
Exported Nether.Bounds() and Nether.Deactivate() are unused
Low Severity
Nether.Bounds() and Nether.Deactivate() are exported but never called anywhere in the codebase. Portal deactivation is handled exclusively through the package-level DeactivateNetherPortal function, which uses flood-fill via connectedNetherPortal rather than the Nether struct's stored positions. These methods are dead code on a newly introduced type.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8944d58. Configure here.
HashimTheArab
force-pushed
the
feature/portal
branch
from
0f72991 to
8944d58
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/world/portal/end_test.go (1)
27-49: ⚡ Quick winUse an independent ring fixture instead of mirroring the production math.
This helper rebuilds the same tangent/step logic as
expectedEndRingFrames(), so a shared coordinate bug can still make the tests pass. Hard-coding one canonical 12-frame layout would give these tests an actual external oracle.🤖 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/world/portal/end_test.go` around lines 27 - 49, The test helper endPortalRingFrames currently duplicates the production tangent/step math and should be replaced with an independent canonical fixture: remove the loop-based construction and instead return a hard-coded slice of 12 ringFrame values (using ringFrame{pos: <cube.Pos offset from center>, facing: <cube.Direction>} entries) that represent the canonical 3x3 interior portal layout around the provided center; keep the function signature endPortalRingFrames(center cube.Pos) []ringFrame and use cube.Pos offsets relative to center when building each ringFrame so tests no longer mirror expectedEndRingFrames() logic.
🤖 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/register.go`:
- Around line 292-295: The EnderEye item type (EnderEye) was added but not
registered, so decoding/spawning "minecraft:ender_eye" fails; fix this by adding
a registration call in the same registry block where other end items are
registered—insert world.RegisterItem(EnderEye{}) alongside
world.RegisterItem(EndBricks{}), world.RegisterItem(EndPortal{}), etc., so the
item registry knows how to construct/deserialize the EnderEye type.
In `@server/entity/travel_test.go`:
- Around line 294-309: The helper entityInWorld currently treats the 50ms probe
timeout as definitive "false"; change entityInWorld to return (bool, error)
instead of bool, keep the goroutine/ExecWorld logic the same, but on timeout
return (false, fmt.Errorf("timeout checking entity in world after 50ms")) so
callers can distinguish "not in this world" vs "probe timed out" and then update
test callers to fail the test on non-nil error or handle the tri-state
accordingly.
In `@server/entity/travel.go`:
- Around line 28-29: The pending transfer currently only stores the destination
world, causing travel() to recompute the source from the entity's current
position later; update the pending state to also capture the portal-entry
coordinates (e.g., add a sourcePos field on the pending struct), set that
sourcePos when queuing a portal transfer, and modify finishPendingPortalTravel()
to pass the stored sourcePos into travel()/the transfer path instead of calling
e.Position(); update any other places that queue or resolve pending transfers
(see code around the current pending declaration and usages in
finishPendingPortalTravel() and travel()) to read/write the new sourcePos field.
---
Nitpick comments:
In `@server/world/portal/end_test.go`:
- Around line 27-49: The test helper endPortalRingFrames currently duplicates
the production tangent/step math and should be replaced with an independent
canonical fixture: remove the loop-based construction and instead return a
hard-coded slice of 12 ringFrame values (using ringFrame{pos: <cube.Pos offset
from center>, facing: <cube.Direction>} entries) that represent the canonical
3x3 interior portal layout around the provided center; keep the function
signature endPortalRingFrames(center cube.Pos) []ringFrame and use cube.Pos
offsets relative to center when building each ringFrame so tests no longer
mirror expectedEndRingFrames() logic.
🪄 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: 86e45f0c-3118-4153-affe-37be04274030
📒 Files selected for processing (12)
server/block/end_portal.goserver/block/end_portal_frame.goserver/block/end_portal_test.goserver/block/hash.goserver/block/register.goserver/entity/travel.goserver/entity/travel_test.goserver/item/ender_eye.goserver/item/register.goserver/player/conf.goserver/world/portal/end.goserver/world/portal/end_test.go
✅ Files skipped from review due to trivial changes (2)
- server/item/register.go
- server/block/hash.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/player/conf.go
| world.RegisterItem(EndBricks{}) | ||
| world.RegisterItem(EndPortal{}) | ||
| world.RegisterItem(EndPortalFrame{}) | ||
| world.RegisterItem(EndRod{}) |
There was a problem hiding this comment.
Register EnderEye in the item registry.
item.EnderEye is added in this PR, but it never gets registered here. Without that, "minecraft:ender_eye" cannot be decoded/spawned through the item registry, so the new activation flow only works in tests that instantiate the type directly.
Suggested fix
world.RegisterItem(EndBricks{})
+ world.RegisterItem(EnderEye{})
world.RegisterItem(EndPortal{})
world.RegisterItem(EndPortalFrame{})
world.RegisterItem(EndRod{})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| world.RegisterItem(EndBricks{}) | |
| world.RegisterItem(EndPortal{}) | |
| world.RegisterItem(EndPortalFrame{}) | |
| world.RegisterItem(EndRod{}) | |
| world.RegisterItem(EndBricks{}) | |
| world.RegisterItem(EnderEye{}) | |
| world.RegisterItem(EndPortal{}) | |
| world.RegisterItem(EndPortalFrame{}) | |
| world.RegisterItem(EndRod{}) |
🤖 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/register.go` around lines 292 - 295, The EnderEye item type
(EnderEye) was added but not registered, so decoding/spawning
"minecraft:ender_eye" fails; fix this by adding a registration call in the same
registry block where other end items are registered—insert
world.RegisterItem(EnderEye{}) alongside world.RegisterItem(EndBricks{}),
world.RegisterItem(EndPortal{}), etc., so the item registry knows how to
construct/deserialize the EnderEye type.
| func entityInWorld(handle *world.EntityHandle, w *world.World) bool { | ||
| result := make(chan bool, 1) | ||
| go func() { | ||
| var ok bool | ||
| running := handle.ExecWorld(func(tx *world.Tx, _ world.Entity) { | ||
| ok = tx.World() == w | ||
| }) | ||
| result <- running && ok | ||
| }() | ||
|
|
||
| select { | ||
| case ok := <-result: | ||
| return ok | ||
| case <-time.After(50 * time.Millisecond): | ||
| return false | ||
| } |
There was a problem hiding this comment.
Don't treat probe timeouts as proof the entity is gone.
Line 307 collapses “could not verify within 50ms” into false, and the callers use that as a definitive absence check. That can make the source-world assertions pass spuriously and hide stuck probes under load. Fail the test on timeout, or return a tri-state/result+error so callers can distinguish “not in this world” from “timed out checking”.
🤖 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/entity/travel_test.go` around lines 294 - 309, The helper
entityInWorld currently treats the 50ms probe timeout as definitive "false";
change entityInWorld to return (bool, error) instead of bool, keep the
goroutine/ExecWorld logic the same, but on timeout return (false,
fmt.Errorf("timeout checking entity in world after 50ms")) so callers can
distinguish "not in this world" vs "probe timed out" and then update test
callers to fail the test on non-nil error or handle the tri-state accordingly.
| pending *world.World | ||
| } |
There was a problem hiding this comment.
Capture the portal-entry position for queued transfers.
pending only remembers the destination world. When finishPendingPortalTravel() runs later in the tick, travel() recomputes the source coordinates from the entity's current position instead of the position that actually triggered portal travel. For callers that keep moving after touching the portal, that can send the Nether/Overworld mapping through the wrong coordinate pair.
Suggested direction
type PortalTravelComputer struct {
...
- pending *world.World
+ pending *queuedPortalTravel
}
+
+type queuedPortalTravel struct {
+ destination *world.World
+ sourcePos cube.Pos
+}
-func (t *PortalTravelComputer) queuePortalTravel(tx *world.Tx, target world.Dimension) {
+func (t *PortalTravelComputer) queuePortalTravel(sourcePos cube.Pos, tx *world.Tx, target world.Dimension) {
if destination := t.enterPortal(tx, target); destination != nil {
t.mu.Lock()
- t.pending = destination
+ t.pending = &queuedPortalTravel{destination: destination, sourcePos: sourcePos}
t.mu.Unlock()
}
}Then have finishPendingPortalTravel() pass the stored sourcePos into the transfer path instead of recalculating it from e.Position().
Also applies to: 59-64, 106-117
🤖 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/entity/travel.go` around lines 28 - 29, The pending transfer currently
only stores the destination world, causing travel() to recompute the source from
the entity's current position later; update the pending state to also capture
the portal-entry coordinates (e.g., add a sourcePos field on the pending
struct), set that sourcePos when queuing a portal transfer, and modify
finishPendingPortalTravel() to pass the stored sourcePos into travel()/the
transfer path instead of calling e.Position(); update any other places that
queue or resolve pending transfers (see code around the current pending
declaration and usages in finishPendingPortalTravel() and travel()) to
read/write the new sourcePos field.
Constraint: Preserve portal insider scan bounds while reducing bespoke loop code. Confidence: high Scope-risk: narrow Directive: Keep portal contact checks on the entity tick transaction path. Tested: go test ./server/entity; go test ./server/block/cube Not-tested: full repository test suite
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit baea218. Configure here.
| return destination | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing nil check allows unnecessary portal state mutation
Medium Severity
enterPortal checks destination == source but not destination == nil. When PortalDestination returns nil (no destination configured), nil != source passes the guard, allowing the function to set inside = true and start the awaitingTravel timer for non-instantaneous entities. After 4 seconds the timer fires, enterPortal returns the nil destination, and the caller correctly skips travel — but awaitingTravel remains true and never self-resets while the entity stays inside the portal, causing enterPortal to execute the full timer-check path every tick instead of short-circuiting.
Reviewed by Cursor Bugbot for commit baea218. Configure here.
| return mgl64.Vec3{1, 0, 0} | ||
| } | ||
| panic("invalid face") | ||
| } |
There was a problem hiding this comment.
New exported Face.Offset method is never called
Low Severity
The newly added Face.Offset() method is not called anywhere in the codebase. A grep for .Offset() returns no matches, making this dead code that adds surface area to the public API without any consumer.
Reviewed by Cursor Bugbot for commit baea218. Configure here.
3 participants
Note
High Risk
High risk because it adds new inter-dimensional travel, spawns entities into different worlds (
Tx.AddEntityAt/World.addEntityAt), and changes entity tick/teleport flows, which can affect core world/entity lifecycle and concurrency.Overview
Adds a full Nether portal implementation: new
block.Portal(withmodel.Portal), frame detection/activation/deactivation inworld/portal, and registration/hashing so portal blocks can exist in-world.Portal activation is now triggered by fire placement (fire spread/start,
FlintAndSteel,FireCharge), and portal blocks self-clean up when their obsidian frame breaks.Introduces cross-world entity travel via
PortalTravelComputer(queued or terminal transfer, coordinate scaling, portal creation on arrival), integrates it intoEntticks and multiple behaviours, and updates player teleport handling to support portal travel (instant for Creative). AddsTx.AddEntityAt/EntityHandle.setAndUnlockWorldAtto spawn transferred entities at a specific position, plus unit tests and CI workflows now runninggo test ./...on PRs/pushes.Reviewed by Cursor Bugbot for commit baea218. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Tests