feat: add redstone core engine and basic sources#27
Conversation
📝 WalkthroughWalkthroughComplete reimplementation of the redstone system: a new graph-based evaluation engine replaces breadth-first wire-network propagation; power source/relayer/consumer interfaces define block behavior; Lever, RedstoneBlock, RedstoneWire, RedstoneTorch, and Note blocks updated to new model; world integration adds engine tick, transaction redstone methods, and loaded-only block/liquid queries. ChangesRedstone System Engine Overhaul
🎯 4 (Complex) | ⏱️ ~75 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/world/tick.go (1)
332-339:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSuperseded future ticks still accumulate in
queue.ticks.When the same
(pos, block-hash)is pushed farther into the future,furthestTicks[index]is updated but the older future entry stays inqueue.ticksuntil its original due tick. Repeated postponement will grow stale entries in memory, andfromChunk()will also serialize all of them on save. This is especially risky now that redstone can reschedule aggressively. Consider replacing/pruning the older queued entry when advancingfurthestTicks, rather than only skipping it at execution time.🤖 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/tick.go` around lines 332 - 339, The schedule method updates furthestTicks but leaves outdated entries in the ticks slice; modify scheduledTickQueue.schedule to prune existing queued entries for the same scheduledTickIndex before appending the new one: compute index as you already do (scheduledTickIndex{pos,posHash}), then scan/filter queue.ticks in-place and remove any scheduledTick whose pos and bhash equal index.hash (and whose t < resTick or always remove to be safe) to avoid stale duplicates, then set furthestTicks[index]=resTick and append the new scheduledTick; reference functions/types: scheduledTickQueue.schedule, furthestTicks, ticks ([]scheduledTick), scheduledTickIndex, scheduledTick, and fromChunk() (so saved state no longer contains obsolete entries).
🧹 Nitpick comments (4)
server/block/redstone.go (2)
103-103: ⚡ Quick winReuse the
redstonePowerhelper for clamping.Two sites duplicate the clamp logic that was just centralized in
redstone_common.go. Using the helper keeps the redstone semantics defined in one place.♻️ Proposed diff
func (r RedstoneWire) RedstonePowerUpdate(_ cube.Pos, _ *world.Tx, power int) (world.Block, bool) { - power = max(0, min(power, 15)) + power = redstonePower(power) if r.Power == power { return r, false }func (r RedstoneWire) EncodeBlock() (string, map[string]any) { - return "minecraft:redstone_wire", map[string]any{"redstone_signal": int32(max(0, min(r.Power, 15)))} + return "minecraft:redstone_wire", map[string]any{"redstone_signal": int32(redstonePower(r.Power))} }Also applies to: 140-140
🤖 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/redstone.go` at line 103, The file duplicates clamp logic (power = max(0, min(power, 15))) instead of using the centralized helper; replace those inline clamps with a call to the redstonePower helper (i.e., set power = redstonePower(power)) so all redstone clamping uses the shared logic defined in redstone_common.go; update both places in server/block/redstone.go where the clamp appears to call redstonePower instead.
423-430: ⚖️ Poor tradeoffConsider type-based identification for non-conductors.
redstoneLampExplicitNonConductorkeys offEncodeBlocknames like"minecraft:piston"/"minecraft:piston_arm"/"minecraft:observer". If/when those blocks are implemented (or their encoded names ever drift), the lamp's strong-power conduction will silently diverge from intent. Aswitch b.(type)against concrete types (or a tagging interface likeredstoneNonConductor) is more resilient and consistent with howredstoneWireDirectConnectionLoadeddiscriminates relayers/sources by type.That said, given several of these blocks (
Piston,PistonArm,Observer) are not yet in the codebase, the string approach is understandable as a placeholder.🤖 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/redstone.go` around lines 423 - 430, redstoneLampExplicitNonConductor currently matches block identity by EncodeBlock string names which is brittle; change it to use a type-based check instead: either add a marker interface (e.g. redstoneNonConductor) and make concrete types (RedstoneBlock, Piston, StickyPiston, PistonArm, Observer) implement it, then return true when b implements that interface, or perform a type switch on b.(type) inside redstoneLampExplicitNonConductor to match those concrete types directly; this mirrors how redstoneWireDirectConnectionLoaded discriminates and prevents future name-drift bugs.server/block/register.go (1)
517-541: 💤 Low valueOptional consolidation: merge the single-element loop with the wood-type loop.
The polished-blackstone group registers exactly the same
Button+PressurePlatepair as the wood types — they could share one loop, with the weighted pressure plates left as standalone (since they have noButtonequivalent). Purely cosmetic; safe to defer.♻️ Suggested consolidation
- for _, typ := range []int{ - redstoneSourcePolishedBlackstone, - } { - world.RegisterItem(Button{Type: typ}) - world.RegisterItem(PressurePlate{Type: typ}) - } world.RegisterItem(PressurePlate{Type: redstoneSourceLightWeighted}) world.RegisterItem(PressurePlate{Type: redstoneSourceHeavyWeighted}) for _, typ := range []int{ + redstoneSourcePolishedBlackstone, redstoneSourceOak, redstoneSourceSpruce,🤖 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 517 - 541, Consolidate the duplicated single-element loop by adding redstoneSourcePolishedBlackstone into the existing wood-type slice that registers Button and PressurePlate so both cases use the same loop that calls world.RegisterItem(Button{Type: typ}) and world.RegisterItem(PressurePlate{Type: typ}); leave the two weighted pressure plate registrations (PressurePlate{Type: redstoneSourceLightWeighted} and PressurePlate{Type: redstoneSourceHeavyWeighted}) as standalone lines because they have no corresponding Button. This touches the loops referencing redstoneSourcePolishedBlackstone and the wood-type slice that iterates over redstoneSourceOak, redstoneSourceSpruce, etc., and preserves existing RegisterItem calls for Button and PressurePlate.cmd/blockhash/main.go (1)
264-267: 💤 Low valueMinor: assert receiver was actually split out.
strings.Cut(s, ".")returns(s, "", false)when there is no.ins. Todaysis always built asrecvName + "." + fieldName.Name, so the contract holds — but if a future refactor passes a bare identifier, the generated code becomesleverAxisHash(<full-expression>)which may compile but hash incorrectly. A small assertion using the third return makes the contract explicit.♻️ Suggested guard
case "Axis": if _, ok := directives["lever_axis"]; ok { - receiver, _, _ := strings.Cut(s, ".") + receiver, _, found := strings.Cut(s, ".") + if !found { + log.Fatalf("lever_axis directive on field %q: expected qualified expression", s) + } return "leverAxisHash(" + receiver + ")", 1 } return "uint64(" + s + ")", 2🤖 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 `@cmd/blockhash/main.go` around lines 264 - 267, Change the strings.Cut call to capture the boolean and assert it succeeded: use receiver, _, ok := strings.Cut(s, ".") and if ok is false panic/log.Fatalf with a clear message like "expected receiver.field, got %q" (using s) so the contract is explicit before returning "leverAxisHash(" + receiver + ")". This targets the strings.Cut usage in the lever_axis handling block.
🤖 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/redstone_torch.go`:
- Around line 154-163: torchFacingDirection currently returns the attachment
face instead of the torch head direction; change torchFacingDirection(face
cube.Face) to return face.Opposite().String() for wall faces (keep FaceDown ->
"top"), update the decoder that parses "torch_facing_direction" back into block
Facing to apply .Opposite() when mapping head-direction strings to cube.Face so
decode/encode are symmetric, update unit tests TestRedstoneTorchRegisteredStates
and TestRedstoneTorchPowerAndEncode to expect the opposite mapping (e.g.,
"torch_facing_direction":"west" -> Facing: cube.FaceEast), and make the same
fixes in torch.go and copper_torch.go to ensure all torch types use the
head-direction encoding consistent with Bedrock.
In `@server/world/redstone.go`:
- Around line 250-285: compileRegion's BFS uses e.redstoneRelayerNeighbours
(which iterates only 6 faces and ignores the Block) so it misses custom relayer
connections (e.g., upward-step wires); change compileRegion to call
e.redstoneRelayerNeighbourPositions(tx, p, b) and enqueue each returned cube.Pos
(after checking blockLoaded/isRedstoneRelevant) so the BFS follows the same
neighbor rules used by compileEdges and powerFrom; after this, remove or make
redstoneRelayerNeighbours delegate to redstoneRelayerNeighbourPositions to avoid
drift, and likewise update invalidateAround to mark dirty positions using
redstoneRelayerNeighbourPositions so custom neighbor relations propagate
dirtiness.
In `@server/world/world.go`:
- Around line 161-170: blockLoaded must not call blockInChunk because that
method lazily creates NBT block entities and triggers ViewBlockUpdate; instead
read the block value directly from the chunk without side effects. Replace the
call to blockInChunk(c, pos) with a non-mutating lookup: compute the block's
local coordinates from pos, read the block ID/state from the chunk's raw storage
or a new chunk method that only returns data (e.g., chunk.BlockAt/BlockValue or
add World.blockInChunkNoSideEffects), and return that without creating entities
or broadcasting updates. Ensure the new lookup path is used only for loaded-only
reads and does not call any code that may allocate NBT or emit ViewBlockUpdate.
---
Outside diff comments:
In `@server/world/tick.go`:
- Around line 332-339: The schedule method updates furthestTicks but leaves
outdated entries in the ticks slice; modify scheduledTickQueue.schedule to prune
existing queued entries for the same scheduledTickIndex before appending the new
one: compute index as you already do (scheduledTickIndex{pos,posHash}), then
scan/filter queue.ticks in-place and remove any scheduledTick whose pos and
bhash equal index.hash (and whose t < resTick or always remove to be safe) to
avoid stale duplicates, then set furthestTicks[index]=resTick and append the new
scheduledTick; reference functions/types: scheduledTickQueue.schedule,
furthestTicks, ticks ([]scheduledTick), scheduledTickIndex, scheduledTick, and
fromChunk() (so saved state no longer contains obsolete entries).
---
Nitpick comments:
In `@cmd/blockhash/main.go`:
- Around line 264-267: Change the strings.Cut call to capture the boolean and
assert it succeeded: use receiver, _, ok := strings.Cut(s, ".") and if ok is
false panic/log.Fatalf with a clear message like "expected receiver.field, got
%q" (using s) so the contract is explicit before returning "leverAxisHash(" +
receiver + ")". This targets the strings.Cut usage in the lever_axis handling
block.
In `@server/block/redstone.go`:
- Line 103: The file duplicates clamp logic (power = max(0, min(power, 15)))
instead of using the centralized helper; replace those inline clamps with a call
to the redstonePower helper (i.e., set power = redstonePower(power)) so all
redstone clamping uses the shared logic defined in redstone_common.go; update
both places in server/block/redstone.go where the clamp appears to call
redstonePower instead.
- Around line 423-430: redstoneLampExplicitNonConductor currently matches block
identity by EncodeBlock string names which is brittle; change it to use a
type-based check instead: either add a marker interface (e.g.
redstoneNonConductor) and make concrete types (RedstoneBlock, Piston,
StickyPiston, PistonArm, Observer) implement it, then return true when b
implements that interface, or perform a type switch on b.(type) inside
redstoneLampExplicitNonConductor to match those concrete types directly; this
mirrors how redstoneWireDirectConnectionLoaded discriminates and prevents future
name-drift bugs.
In `@server/block/register.go`:
- Around line 517-541: Consolidate the duplicated single-element loop by adding
redstoneSourcePolishedBlackstone into the existing wood-type slice that
registers Button and PressurePlate so both cases use the same loop that calls
world.RegisterItem(Button{Type: typ}) and world.RegisterItem(PressurePlate{Type:
typ}); leave the two weighted pressure plate registrations (PressurePlate{Type:
redstoneSourceLightWeighted} and PressurePlate{Type:
redstoneSourceHeavyWeighted}) as standalone lines because they have no
corresponding Button. This touches the loops referencing
redstoneSourcePolishedBlackstone and the wood-type slice that iterates over
redstoneSourceOak, redstoneSourceSpruce, etc., and preserves existing
RegisterItem calls for Button and PressurePlate.
🪄 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: 28553d36-e628-401e-a363-dc9405ec5a96
📒 Files selected for processing (27)
cmd/blockhash/main.goserver/block/hash.goserver/block/lever.goserver/block/note.goserver/block/redstone.goserver/block/redstone_block.goserver/block/redstone_common.goserver/block/redstone_sources.goserver/block/redstone_sources_test.goserver/block/redstone_test.goserver/block/redstone_torch.goserver/block/redstone_torch_test.goserver/block/redstone_wire.goserver/block/register.goserver/session/world.goserver/world/block.goserver/world/conf.goserver/world/handler.goserver/world/redstone.goserver/world/redstone/state.goserver/world/redstone_bench_test.goserver/world/redstone_test.goserver/world/sound/block.goserver/world/tick.goserver/world/tx.goserver/world/world.goserver/world/world_test.go
💤 Files with no reviewable changes (5)
- server/block/lever.go
- server/block/redstone_block.go
- server/block/redstone_wire.go
- server/world/block.go
- server/world/redstone/state.go
| func torchFacingDirection(face cube.Face) string { | ||
| switch face { | ||
| case cube.FaceDown: | ||
| return "top" | ||
| case unknownFace: | ||
| return "unknown" | ||
| default: | ||
| return face.String() | ||
| } | ||
| return 0, false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show the torch test and any related decode-side mapping.
fd -t f 'redstone_torch_test.go' --exec cat {}
# Search for any decoding of torch_facing_direction in the repo (e.g. nbt/state parsers).
rg -nP -C3 'torch_facing_direction'
# Also pull the previous (pre-PR) torch encoding for comparison.
git log --oneline -n 5 -- server/block/redstone_torch.goRepository: HashimTheArab/dragonfly
Length of output: 7222
🏁 Script executed:
# Find where torch_facing_direction is decoded back to Facing
rg -n "torch_facing_direction" --type go -B 5 -A 5 | head -100
# Look for RegisteredStates or block registration that maps properties to block structs
fd -t f 'redstone_torch.go' --exec grep -n "func.*torch\|RegisteredStates\|StateProperties" {} +
# Check full redstone_torch.go file structure
wc -l server/block/redstone_torch.goRepository: HashimTheArab/dragonfly
Length of output: 5662
🏁 Script executed:
# Get full redstone_torch.go to understand the decoding side
cat server/block/redstone_torch.goRepository: HashimTheArab/dragonfly
Length of output: 4723
🏁 Script executed:
# Search for where RedstoneTorch states are registered/decoded
rg -n "RegisteredStates\|parseFrom\|newRedstoneTorch" --type go
# Search for where torch_facing_direction is actually parsed back to Facing
rg -n "torch_facing_direction.*west\|torch_facing_direction.*north" --type go -B 3 -A 3
# Look for state parsing logic related to blocks
fd -t f 'state.go\|states.go' --exec grep -l "RedstoneTorch\|redstone_torch" {} \;Repository: HashimTheArab/dragonfly
Length of output: 49
🏁 Script executed:
# Try broader search for state registration
rg -n "minecraft:redstone_torch\|minecraft:unlit_redstone_torch" --type go -B 3 -A 3Repository: HashimTheArab/dragonfly
Length of output: 49
🏁 Script executed:
# Find where allRedstoneTorches is used and how blocks are registered
rg -n "allRedstoneTorches" --type go -B 5 -A 5
# Search for init functions or state registration in block package
fd -t f '\.go$' server/block/ --exec grep -l "func init" {} \;
# Look for any files that might handle block state registration
fd -t f '\.go$' server/block/ | head -20Repository: HashimTheArab/dragonfly
Length of output: 2987
🏁 Script executed:
# Search for RegisteredStates or similar patterns in block package
find server/block -name "*.go" -exec grep -l "RegisteredStates\|register.*block\|func.*states" {} \;Repository: HashimTheArab/dragonfly
Length of output: 128
🏁 Script executed:
# Read register.go to understand block registration
cat server/block/register.go | head -250Repository: HashimTheArab/dragonfly
Length of output: 7979
🏁 Script executed:
# Also search for how state parsing works - look for property mapping
rg -n "func.*state\|properties\|StateProperties" server/block/ --type go | head -30Repository: HashimTheArab/dragonfly
Length of output: 49
🏁 Script executed:
# Check how copper_torch and torch handle encoding and decoding
cat server/block/copper_torch.go | grep -A 20 "func.*copper.*Encode\|allCopperTorches"
# Check torch.go encoding
cat server/block/torch.go | grep -A 20 "func.*Encode\|allTorches"
# Look for cube.Face documentation or comments
rg -n "cube.Face\|Facing" server/block/redstone_torch.go -B 1 -A 1
# Check if there are any comments in the entire torch-related code
rg -n "torch\|Torch" server/block/torch.go | head -20Repository: HashimTheArab/dragonfly
Length of output: 1257
🏁 Script executed:
# Search for test files that might show how torch_facing_direction should work
find . -name "*torch*test*" -type f -exec cat {} \;Repository: HashimTheArab/dragonfly
Length of output: 3297
Fix wall torch direction encoding to match Bedrock semantics—torch head direction is opposite to attachment direction.
torchFacingDirection incorrectly encodes the attachment direction instead of the torch head direction. Bedrock's torch_facing_direction represents where the torch head points (e.g., "west" = torch head points west = attached to east block). Per the legacy Minecraft torch data values, a torch attached to FaceEast (Facing=East) should encode as "west" (torch head direction), but currently encodes as "east" (attachment direction).
The helper should use face.Opposite().String() for wall faces, not face.String(). The FaceDown -> "top" case is correct as-is.
However, this bug fix requires coordinated changes across at least three places:
- redstone_torch.go – Change
torchFacingDirection()to use.Opposite()for wall faces - Decoder logic – Wherever
torch_facing_directionis parsed back toFacing(likely in theworldpackage's state registry), apply.Opposite()as well - Test assertions –
TestRedstoneTorchRegisteredStatesandTestRedstoneTorchPowerAndEncodein redstone_torch_test.go currently expect direct (wrong) mapping. After fixing encoding/decoding, update the test to expect opposite mapping (e.g.,"torch_facing_direction": "west"should decode toFacing: FaceEast, notFaceWest) - torch.go and copper_torch.go – Both use the same buggy
face.String()pattern and likely have the same issue
Verify the fix against actual Bedrock block states and ensure all three torch types are corrected consistently.
🤖 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/redstone_torch.go` around lines 154 - 163, torchFacingDirection
currently returns the attachment face instead of the torch head direction;
change torchFacingDirection(face cube.Face) to return face.Opposite().String()
for wall faces (keep FaceDown -> "top"), update the decoder that parses
"torch_facing_direction" back into block Facing to apply .Opposite() when
mapping head-direction strings to cube.Face so decode/encode are symmetric,
update unit tests TestRedstoneTorchRegisteredStates and
TestRedstoneTorchPowerAndEncode to expect the opposite mapping (e.g.,
"torch_facing_direction":"west" -> Facing: cube.FaceEast), and make the same
fixes in torch.go and copper_torch.go to ensure all torch types use the
head-direction encoding consistent with Bedrock.
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 72496ea. Configure here.
| // EncodeBlock ... | ||
| func (r RedstoneWire) EncodeBlock() (string, map[string]any) { | ||
| return "minecraft:redstone_wire", map[string]any{"redstone_signal": int32(max(0, min(r.Power, 15)))} | ||
| } |
There was a problem hiding this comment.
RedstoneWire lost TrimMaterial and MaterialColour methods
Medium Severity
The new RedstoneWire implementation is missing the TrimMaterial() and MaterialColour() methods that existed on the old RedstoneWire in redstone_wire.go. These methods delegated to item.RedstoneWire{} and were used for smithing trim decoding. Their removal could break trim-related functionality for redstone wire.
Reviewed by Cursor Bugbot for commit 72496ea. Configure here.
| "lever_direction": leverDirection(l.Facing, l.Axis), | ||
| "open_bit": boolByte(l.Powered), | ||
| } | ||
| } |
There was a problem hiding this comment.
Lever EncodeBlock uses uint8 instead of bool for open_bit
Medium Severity
The new Lever.EncodeBlock encodes open_bit as boolByte(l.Powered) (a uint8), whereas the old implementation used l.Powered directly (a bool). In hashProperties, bool and uint8 are both valid but produce the same byte values — however, they are different Go types and take different switch branches. If the registered block states from block_states.nbt store open_bit as bool, the state hash will not match, causing the lever block to fail to resolve its runtime ID.
Reviewed by Cursor Bugbot for commit 72496ea. Configure here.
| torch.Lit = torch.inputStrength(pos, tx) == 0 | ||
| tx.SetBlock(pos, torch, nil) | ||
| updateTorchRedstone(pos, tx) | ||
| return true |
There was a problem hiding this comment.
RedstoneTorch attachmentPowered may create feedback loop
Medium Severity
attachmentPowered first calls tx.RedstonePower(attached) to check power at the attached block, then additionally checks if the attached block itself is a RedstonePowerSource and queries all its faces. This double-check means a RedstoneBlock placed where the torch attaches would be counted twice — once through the engine's powerTo and once through the direct RedstonePowerSource check. More critically, this direct RedstonePowerSource query bypasses the engine's cycle-detection (evaluating guard), potentially creating infinite recursion when two torches indirectly power each other's attachment blocks.
Reviewed by Cursor Bugbot for commit 72496ea. Configure here.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/block/redstone_torch.go (1)
213-222:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
torchFacingDirectionstill encodes attachment direction instead of head direction.This was previously flagged and not addressed. Confirming with Bedrock docs: a
torch_facing_directionvalue of "west" denotes "a torch that is facing West and attached to a block on the East side" — i.e., the property describes the torch head direction, which is the opposite of the attachment direction.In this codebase
t.Facingis documented as "the direction from the torch to the block it is attached to" (the attachment direction), so for wall torches the encoded string should beface.Opposite().String(), notface.String(). TheFaceDown → "top"branch happens to be correct because down's opposite is up = "top". As noted on the prior commit, the decoder side andTestRedstoneTorchRegisteredStates/TestRedstoneTorchPowerAndEncodeneed to flip in lockstep, and the same fix likely applies totorch.go/copper_torch.go.🔧 Suggested encoding fix (decoder + tests must follow)
func torchFacingDirection(face cube.Face) string { switch face { case cube.FaceDown: return "top" case unknownFace: return "unknown" default: - return face.String() + return face.Opposite().String() } }Run the following to locate the decode-side mapping and any other callers that may need a matching
.Opposite()flip:#!/bin/bash rg -nP -C3 'torch_facing_direction' rg -nP -C3 '\btorchFacingDirection\s*\(' # Also confirm parity for torch.go and copper_torch.go. fd -t f -e go 'torch.go|copper_torch.go' --exec rg -nP -C3 'face\.String\(\)' {}🤖 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/redstone_torch.go` around lines 213 - 222, torchFacingDirection currently returns the torch attachment direction (face.String()) but Bedrock expects the torch head direction (the opposite); update torchFacingDirection to return face.Opposite().String() for wall faces (keep the FaceDown -> "top" behavior as is since opposite of Down is Up/"top"), and then update any decode-side mapping and all callers that assume the old encoding (including torch.go, copper_torch.go, and the decoder mapping for torch_facing_direction) so encode/decode remain symmetric; also update tests TestRedstoneTorchRegisteredStates and TestRedstoneTorchPowerAndEncode to expect the flipped value.
🧹 Nitpick comments (3)
server/block/redstone_wire.go (1)
65-73: ⚖️ Poor tradeoffOptional: Cache the powered-faces map per query batch.
RedstonePowerinvokesredstoneWirePowersHorizontalFacefor each queried face, and that in turn rebuilds the full powered-horizontal-faces map (4 directions × direct/up-step/down-step probes — each doing severalBlockLoadedlookups). If the engine queries multiple faces on the same wire during one evaluation pass, the same map is recomputed every call.Not a correctness issue, and the current implementation prioritizes clarity, but worth keeping in mind if redstone evaluation shows up as hot in a profile — a per-evaluation cache keyed by
(pos, engineEpoch)(or simply computing the map once at the relayer evaluation boundary and passing it through) would eliminate the redundancy.Also applies to: 140-162
🤖 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/redstone_wire.go` around lines 65 - 73, RedstonePower repeatedly calls redstoneWirePowersHorizontalFace and rebuilds the same powered-horizontal-faces map for each face queried, causing redundant BlockLoaded checks; to fix, compute and reuse that map per evaluation batch by adding a short-lived cache keyed by the wire position (pos) and the current evaluation epoch (or by computing the map once at the relayer/evaluation boundary and passing it down), then have RedstonePower accept or fetch the cached map and use it instead of calling redstoneWirePowersHorizontalFace each time; update callers and helper functions (RedstonePower, redstoneWirePowersHorizontalFace, and any relayer/evaluation entry point) to pass or read the cache so the map is computed only once per evaluation.server/block/redstone_test.go (2)
260-265: ⚡ Quick winAvoid hardcoding burnout pre-threshold toggles in this integration test.
Using a fixed count (
7) couples the test to internal threshold tuning and can create brittle failures unrelated to behavior.More robust toggle loop
- for i := 0; i < 7; i++ { - if tx.RecordRedstoneTorchToggle(pos) { - err = fmt.Errorf("torch burned out after %d toggles, want below threshold", i+1) - return - } - } + burnedOut := false + for i := 0; i < 64; i++ { + if tx.RecordRedstoneTorchToggle(pos) { + burnedOut = true + break + } + } + if !burnedOut { + err = fmt.Errorf("torch did not burn out within expected toggle budget") + 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/block/redstone_test.go` around lines 260 - 265, Don't hardcode 7; instead query the burnout threshold constant and iterate up to threshold-1 so the test remains stable if tuning changes. Replace the fixed loop bound with the package constant (e.g. redstone.TorchBurnoutThreshold or the equivalent symbol used in the codebase) and loop for threshold-1 iterations calling tx.RecordRedstoneTorchToggle(pos), ensuring you update imports if needed and keep the existing error check that fails if a toggle returns true before the threshold.
85-107: ⚡ Quick winAdd a negative-power clamp assertion in wire update coverage.
This test validates upper clamping but misses the lower bound (
< 0 -> 0), so a regression there could slip through unnoticed.Proposed test addition
func TestRedstoneWirePowerUpdate(t *testing.T) { wire := RedstoneWire{Power: 3} after, changed := wire.RedstonePowerUpdate(cube.Pos{}, nil, 12) @@ if got := after.(RedstoneWire).Power; got != 15 { t.Fatalf("RedstoneWire clamped power = %d, want 15", got) } + + after, changed = wire.RedstonePowerUpdate(cube.Pos{}, nil, -2) + if !changed { + t.Fatal("RedstoneWire negative clamped update did not report a change") + } + if got := after.(RedstoneWire).Power; got != 0 { + t.Fatalf("RedstoneWire negative clamped power = %d, want 0", got) + } _, changed = (RedstoneWire{Power: 15}).RedstonePowerUpdate(cube.Pos{}, nil, 15) if changed {🤖 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/redstone_test.go` around lines 85 - 107, The test TestRedstoneWirePowerUpdate currently checks upper clamping but not lower clamping; add an assertion that feeding a negative power into RedstoneWire.RedstonePowerUpdate clamps to 0. Specifically, call RedstonePowerUpdate on a RedstoneWire (e.g., Power: 3 or 15) with a negative desired power (e.g., -5), assert changed is true (unless unchanged expected), and assert the returned after.(RedstoneWire).Power == 0 to ensure the lower bound is enforced.
🤖 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/block/redstone_torch.go`:
- Around line 213-222: torchFacingDirection currently returns the torch
attachment direction (face.String()) but Bedrock expects the torch head
direction (the opposite); update torchFacingDirection to return
face.Opposite().String() for wall faces (keep the FaceDown -> "top" behavior as
is since opposite of Down is Up/"top"), and then update any decode-side mapping
and all callers that assume the old encoding (including torch.go,
copper_torch.go, and the decoder mapping for torch_facing_direction) so
encode/decode remain symmetric; also update tests
TestRedstoneTorchRegisteredStates and TestRedstoneTorchPowerAndEncode to expect
the flipped value.
---
Nitpick comments:
In `@server/block/redstone_test.go`:
- Around line 260-265: Don't hardcode 7; instead query the burnout threshold
constant and iterate up to threshold-1 so the test remains stable if tuning
changes. Replace the fixed loop bound with the package constant (e.g.
redstone.TorchBurnoutThreshold or the equivalent symbol used in the codebase)
and loop for threshold-1 iterations calling tx.RecordRedstoneTorchToggle(pos),
ensuring you update imports if needed and keep the existing error check that
fails if a toggle returns true before the threshold.
- Around line 85-107: The test TestRedstoneWirePowerUpdate currently checks
upper clamping but not lower clamping; add an assertion that feeding a negative
power into RedstoneWire.RedstonePowerUpdate clamps to 0. Specifically, call
RedstonePowerUpdate on a RedstoneWire (e.g., Power: 3 or 15) with a negative
desired power (e.g., -5), assert changed is true (unless unchanged expected),
and assert the returned after.(RedstoneWire).Power == 0 to ensure the lower
bound is enforced.
In `@server/block/redstone_wire.go`:
- Around line 65-73: RedstonePower repeatedly calls
redstoneWirePowersHorizontalFace and rebuilds the same powered-horizontal-faces
map for each face queried, causing redundant BlockLoaded checks; to fix, compute
and reuse that map per evaluation batch by adding a short-lived cache keyed by
the wire position (pos) and the current evaluation epoch (or by computing the
map once at the relayer/evaluation boundary and passing it down), then have
RedstonePower accept or fetch the cached map and use it instead of calling
redstoneWirePowersHorizontalFace each time; update callers and helper functions
(RedstonePower, redstoneWirePowersHorizontalFace, and any relayer/evaluation
entry point) to pass or read the cache so the map is computed only once per
evaluation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45b2ab54-c41f-48cf-9c33-ceb1b79bd5e7
📒 Files selected for processing (13)
server/block/lever.goserver/block/redstone.goserver/block/redstone_block.goserver/block/redstone_test.goserver/block/redstone_torch.goserver/block/redstone_wire.goserver/world/redstone.goserver/world/redstone_test.goserver/world/redstone_torch.goserver/world/tick.goserver/world/tx.goserver/world/world.goserver/world/world_test.go
✅ Files skipped from review due to trivial changes (1)
- server/world/world_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/world/redstone.go
- server/world/world.go
2 participants
Note
High Risk
High risk because it replaces core redstone power propagation/interfaces and integrates a new redstone engine into world ticking, block updates, and handler APIs, which can affect many blocks and update ordering/performance.
Overview
Introduces a new graph-based redstone engine (
server/world/redstone.go) that compiles redstone-relevant regions, propagates power (including strong power conduction), and applies cancellable block-state/action updates via newworld.Redstone*interfaces.Refactors redstone blocks to use the new engine and adds new components:
Button,PressurePlate, andRedstoneLamp; updatesLever,RedstoneTorch,RedstoneWire,RedstoneBlock, andNoteto the newRedstonePowerUpdate-style APIs, with extensive new unit tests for power rules, scheduling behavior, and cancellation.Wires the engine into world lifecycle: world tick now runs a redstone phase,
SetBlock/SetLiquidinvalidate redstone unless opted out (DisableRedstoneUpdates), scheduled tick queue semantics were tightened, chunk close cleans up redstone state, andTxgainsBlockLoaded/LiquidLoadedplus redstone query helpers. Handler API is extended with optionalRedstoneHandlercarrying richRedstoneUpdatemetadata while preserving legacy callbacks, and additional redstone-related sounds/events are mapped.Reviewed by Cursor Bugbot for commit 72496ea. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Improvements