Implement vanilla shield blocking#23
Conversation
📝 WalkthroughWalkthroughAdds shield blocking end-to-end: new Shield item and sound, expanded explosion/damage schemas with source/blockability, TNT source propagation and blockability, projectile deflection and marking, player shield mechanics and state integration, session input/metadata wiring, inventory held-item updates, registry fallback, and many tests. ChangesShield Blocking Mechanics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
HashimTheArab
force-pushed
the
shields-vanilla-df-master
branch
5 times, most recently
from
0e8dae4 to
4b9d95e
Compare
HashimTheArab
force-pushed
the
shields-vanilla-df-master
branch
from
4b9d95e to
e6e1954
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
server/entity/projectile_test.go (2)
38-43: ⚡ Quick winUse the new
Ent.MarkShieldBlocked()helper for clarity.Production code is expected to call shield blocking via the
Ent.MarkShieldBlocked()helper added inserver/entity/ent.go. The test currently bypasses that helper with a structural type-assertion chain. Switching to the helper makes the test mirror production usage and one less place to update ifBehaviour()plumbing changes.♻️ Suggested simplification
func (t *projectileShieldTarget) Hurt(_ float64, src world.DamageSource) (float64, bool) { if s, ok := src.(ProjectileDamageSource); ok && t.blocked { - s.Projectile.(*Ent).Behaviour().(interface{ MarkShieldBlocked() }).MarkShieldBlocked() + s.Projectile.(*Ent).MarkShieldBlocked() } return 0, t.vulnerable }🤖 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_test.go` around lines 38 - 43, The test's Hurt method uses a structural type assertion to call the shield-blocking behavior; replace that chain with the new helper by calling Ent.MarkShieldBlocked() on the projectile instance: inside projectileShieldTarget.Hurt, after confirming src is a ProjectileDamageSource and t.blocked, cast s.Projectile to *Ent and invoke its MarkShieldBlocked() method (instead of s.Projectile.(*Ent).Behaviour().(interface{ MarkShieldBlocked() }).MarkShieldBlocked()), so the test mirrors production usage and centralizes the behavior.
83-88: 💤 Low valueManually-built
EntskipsOpenwiring.
newProjectileShieldTestEntconstructs anEntdirectly rather than going throughOpen(tx, handle, data). This works for the two tests that only invokebehaviour.hitEntity(which never touchese.tx), but it would silently break if those tests evolve to call any method that dereferencese.tx(e.g.,Close,SetNameTag,SetOnFirewith a state change). Worth a comment to flag that this builder intentionally returns a "tx-less" Ent for direct unit testing ofhitEntity.🤖 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_test.go` around lines 83 - 88, newProjectileShieldTestEnt constructs an Ent directly without calling Open(tx, handle, data), so the returned Ent has no transaction (e.tx nil) which is fine for tests that only call behaviour.hitEntity but will silently break if tests later call methods that dereference e.tx (e.g., Close, SetNameTag, SetOnFire). Update newProjectileShieldTestEnt (or add an inline comment above it) to explicitly document that it intentionally returns a "tx-less" Ent for direct unit testing of behaviour.hitEntity, or alternatively change the helper to call Ent.Open(tx, handle, data) so the built Ent has a proper tx; reference the newProjectileShieldTestEnt function, Ent type, Open(tx,...), and e.tx when making the change.server/entity/damage.go (1)
49-58: 💤 Low valueOptional: consider
*mgl64.Vec3over theOrigin/HasOriginpair.The
HasOriginflag exists only to disambiguate "no origin set" from "origin at world zero". A*mgl64.Vec3would encode the same nullability without an extra field, but since the rest of the entity API consistently passesmgl64.Vec3by value, the current pattern is also reasonable. Feel free to keep as-is.🤖 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/damage.go` around lines 49 - 58, The Origin/HasOrigin pair in ExplosionDamageSource should be replaced with a nullable pointer to avoid the extra boolean: change the struct field(s) so Origin is of type *mgl64.Vec3 (remove HasOrigin), update all call sites and consumers of ExplosionDamageSource (constructors, uses in functions/methods, and any comparisons) to treat Origin == nil as “no origin” and otherwise dereference the pointer for the Vec3 value, and ensure any code that previously read HasOrigin now checks for Origin != nil; reference ExplosionDamageSource, Origin and HasOrigin to locate and update the affected code paths.server/entity/projectile.go (2)
286-289: ⚖️ Poor tradeoffFragile return-channel via
lt.shieldBlocked.This pattern resets
lt.shieldBlockedtofalse, callsl.Hurt(...), and then inspectslt.shieldBlockedto detect a deflection, relying onHurt's callees synchronously callingMarkShieldBlocked()on this veryProjectileBehaviour. Issues:
- Couples
ProjectileBehaviourmutable state to a transient per-call signal, which is hard to reason about and easy to break ifHurtever becomes asynchronous, calls the projectile multiple times, or re-enters.MarkShieldBlockedbecomes part of the public API only as a side-channel.- Concurrent ticks on different threads with shared behaviour pointers (unlikely today, but not guaranteed forever) would race.
A more robust approach is to surface the shield-block decision through the damage source /
Hurtreturn, e.g., aShieldedboolean as part of the result, or a dedicated source field that the player code sets.🤖 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 286 - 289, The code is using lt.shieldBlocked as a fragile side-channel set by l.Hurt; change the API so l.Hurt (or the damage result it returns) includes an explicit shielded/blocked boolean indicating whether the hit was shield-blocked, then stop using lt.shieldBlocked and MarkShieldBlocked as a cross-call mutable flag. Update the call site in ProjectileBehaviour (where lt.shieldBlocked is reset and l.Hurt(dmg, src) is invoked) to capture the new return value (e.g., result.Shielded or a bool) and call lt.deflect(e, vel) only when that explicit flag is true; remove/reset any MarkShieldBlocked usage and any reliance on shared mutable state so the decision is local to the Hurt call return.
316-323: 💤 Low valueEdge case: zero-magnitude reflected vector won't move out of hitbox.
The early return on
vel.Len() == 0is good, but for non-zero but very small velocities the 0.05 unit offset may still leave the projectile inside the blocker's bounding box. Since the velocity is then reversed, on the next tick the projectile moves away from the blocker, so re-collision should not occur in practice. Worth a comment to make the intent of the 0.05 nudge explicit (separation, not full ejection):📝 Suggested clarifying comment
func (lt *ProjectileBehaviour) deflect(e *Ent, vel mgl64.Vec3) { if vel.Len() == 0 { return } reflected := vel.Mul(-1) e.SetVelocity(reflected) + // Nudge slightly along the reflected direction to avoid re-resolving the same + // collision on the next sub-tick before the reversed velocity carries the + // projectile clear of the blocker's bounding box. e.data.Pos = e.Position().Add(reflected.Normalize().Mul(0.05)) }🤖 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 316 - 323, The deflect logic in ProjectileBehaviour.deflect returns early for zero velocity but uses a fixed 0.05 position nudge which is intended as a separation hack rather than a guaranteed full ejection; update the function to add a concise clarifying comment above the reflected/nudge code explaining that reflected := vel.Mul(-1), e.SetVelocity(reflected) and e.data.Pos = Position().Add(reflected.Normalize().Mul(0.05)) use a small separation offset to avoid stuck-in-hitbox re-collisions and that zero-length velocity is already handled by the early return (optionally note why the value 0.05 was chosen and that tiny non-zero velocities will be moved next tick).server/world/entity_test.go (1)
19-68: 💤 Low valueConsider using table-driven tests for better maintainability.
The three test functions have nearly identical structure and differ only in the parameters passed to
TNTWithSource. Consider consolidating them into a single table-driven test to reduce duplication and improve maintainability.♻️ Example table-driven test approach
+func TestEntityRegistryConfigTNTWithSourceFallback(t *testing.T) { + tests := []struct { + name string + source *EntityHandle + allowUnblockable bool + }{ + {"DefaultTNT", nil, true}, + {"SourceAwareTNT", EntitySpawnOpts{}.New(entityRegistryTestType{}, entityRegistryTestType{}), true}, + {"ShieldUnblockableTNT", nil, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + called := false + reg := EntityRegistryConfig{ + TNT: func(opts EntitySpawnOpts, fuse time.Duration) *EntityHandle { + called = true + return opts.New(entityRegistryTestType{}, entityRegistryTestType{}) + }, + }.New([]EntityType{entityRegistryTestType{}}) + + if h := reg.Config().TNTWithSource(EntitySpawnOpts{}, time.Second, tt.source, tt.allowUnblockable); h == nil { + t.Fatal("expected fallback TNTWithSource to create TNT through TNT") + } + if !called { + t.Fatal("expected fallback TNTWithSource to call TNT") + } + }) + } +}🤖 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/entity_test.go` around lines 19 - 68, Consolidate the three nearly identical tests into one table-driven test that iterates over cases varying the TNTWithSource arguments (source nil vs non-nil and shieldUnblockable true/false) and expected behavior; use a single test function (e.g., TestEntityRegistryConfig_TNTWithSource_Fallback_Table) that constructs the same EntityRegistryConfig with the TNT stub (referencing TNT, EntityRegistryConfig, New, EntitySpawnOpts, and TNTWithSource) and for each case calls reg.Config().TNTWithSource(...), asserting non-nil result and that the TNT stub was invoked; keep the existing assertions but drive inputs and expectations from the table to remove duplication.
🤖 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/projectile.go`:
- Around line 192-198: When handling a deflection in tickMovement, capture the
tick-start position (e.g. startPos := e.Position()) before calling tickMovement
and use that startPos to compute the per-tick displacement for the movement
packet; replace the current m.dpos = m.pos.Sub(result.Position()) with m.dpos =
e.Position().Sub(startPos) so Movement.Send() sees the full tick delta, and keep
m.pos/m.vel and m.dvel assignments as they are (use the saved startPos variable
to compute dpos after the deflection branch).
In `@server/entity/tnt.go`:
- Around line 104-105: The code converts fuse := from
tntFuseAndBlockability(data.Data) into uint8 ticks which will wrap for values
>255; update the handling around fuse (from tntFuseAndBlockability and where m
:= map[string]any{"Fuse": uint8(...)} is created) to detect oversized fuse tick
counts (fuse.Milliseconds()/50 > 255) and either clamp the tick value to 255 or
return/reject with an error before NBT encoding; ensure the check uses the same
milliseconds/50 calculation and apply the sanitized uint8 value into the map so
no silent wrapping occurs.
In `@server/player/player.go`:
- Around line 1424-1426: The code directly clears the flag shieldBlockingInput
which bypasses the cleanup in SetShieldBlockingInput and leaves
shieldBlockingUseHandled stale; change the path that currently sets
shieldBlockingInput = false to call SetShieldBlockingInput(false) so the
function resets shieldBlockingUseHandled and performs any other teardown
consistently (look for symbols shieldBlockingInput, SetShieldBlockingInput,
shieldBlockingUseHandled, and HandleItemUse to verify the fix).
In `@server/session/entity_metadata.go`:
- Around line 84-86: Replace the non-existent constant
protocol.EntityDataFlagBlocking with protocol.EntityDataFlagBlockedUsingShield
in the shield-blocking metadata path: in the block where you check shieldBlocker
and call m.SetFlag (the code using protocol.EntityDataKeyFlagsTwo and
m.SetFlag), update the constant to EntityDataFlagBlockedUsingShield; also update
the assertions in the tests (entity_metadata_test.go) that reference the old
constant to expect protocol.EntityDataFlagBlockedUsingShield instead.
In `@server/session/handler_player_auth_input.go`:
- Around line 169-179: The shieldBlockingInput function is missing handling for
InputFlagStartSneaking; update the initial condition in shieldBlockingInput to
include flags.Load(packet.InputFlagStartSneaking) so that a StartSneaking-only
packet is treated like a sneak input and triggers the shield warm-up transition.
Locate shieldBlockingInput and add packet.InputFlagStartSneaking to the OR group
with InputFlagSneaking/InputFlagSneakDown/InputFlagSneakCurrentRaw (keeping the
existing wasSneaking/sneaking checks and return values unchanged).
---
Nitpick comments:
In `@server/entity/damage.go`:
- Around line 49-58: The Origin/HasOrigin pair in ExplosionDamageSource should
be replaced with a nullable pointer to avoid the extra boolean: change the
struct field(s) so Origin is of type *mgl64.Vec3 (remove HasOrigin), update all
call sites and consumers of ExplosionDamageSource (constructors, uses in
functions/methods, and any comparisons) to treat Origin == nil as “no origin”
and otherwise dereference the pointer for the Vec3 value, and ensure any code
that previously read HasOrigin now checks for Origin != nil; reference
ExplosionDamageSource, Origin and HasOrigin to locate and update the affected
code paths.
In `@server/entity/projectile_test.go`:
- Around line 38-43: The test's Hurt method uses a structural type assertion to
call the shield-blocking behavior; replace that chain with the new helper by
calling Ent.MarkShieldBlocked() on the projectile instance: inside
projectileShieldTarget.Hurt, after confirming src is a ProjectileDamageSource
and t.blocked, cast s.Projectile to *Ent and invoke its MarkShieldBlocked()
method (instead of s.Projectile.(*Ent).Behaviour().(interface{
MarkShieldBlocked() }).MarkShieldBlocked()), so the test mirrors production
usage and centralizes the behavior.
- Around line 83-88: newProjectileShieldTestEnt constructs an Ent directly
without calling Open(tx, handle, data), so the returned Ent has no transaction
(e.tx nil) which is fine for tests that only call behaviour.hitEntity but will
silently break if tests later call methods that dereference e.tx (e.g., Close,
SetNameTag, SetOnFire). Update newProjectileShieldTestEnt (or add an inline
comment above it) to explicitly document that it intentionally returns a
"tx-less" Ent for direct unit testing of behaviour.hitEntity, or alternatively
change the helper to call Ent.Open(tx, handle, data) so the built Ent has a
proper tx; reference the newProjectileShieldTestEnt function, Ent type,
Open(tx,...), and e.tx when making the change.
In `@server/entity/projectile.go`:
- Around line 286-289: The code is using lt.shieldBlocked as a fragile
side-channel set by l.Hurt; change the API so l.Hurt (or the damage result it
returns) includes an explicit shielded/blocked boolean indicating whether the
hit was shield-blocked, then stop using lt.shieldBlocked and MarkShieldBlocked
as a cross-call mutable flag. Update the call site in ProjectileBehaviour (where
lt.shieldBlocked is reset and l.Hurt(dmg, src) is invoked) to capture the new
return value (e.g., result.Shielded or a bool) and call lt.deflect(e, vel) only
when that explicit flag is true; remove/reset any MarkShieldBlocked usage and
any reliance on shared mutable state so the decision is local to the Hurt call
return.
- Around line 316-323: The deflect logic in ProjectileBehaviour.deflect returns
early for zero velocity but uses a fixed 0.05 position nudge which is intended
as a separation hack rather than a guaranteed full ejection; update the function
to add a concise clarifying comment above the reflected/nudge code explaining
that reflected := vel.Mul(-1), e.SetVelocity(reflected) and e.data.Pos =
Position().Add(reflected.Normalize().Mul(0.05)) use a small separation offset to
avoid stuck-in-hitbox re-collisions and that zero-length velocity is already
handled by the early return (optionally note why the value 0.05 was chosen and
that tiny non-zero velocities will be moved next tick).
In `@server/world/entity_test.go`:
- Around line 19-68: Consolidate the three nearly identical tests into one
table-driven test that iterates over cases varying the TNTWithSource arguments
(source nil vs non-nil and shieldUnblockable true/false) and expected behavior;
use a single test function (e.g.,
TestEntityRegistryConfig_TNTWithSource_Fallback_Table) that constructs the same
EntityRegistryConfig with the TNT stub (referencing TNT, EntityRegistryConfig,
New, EntitySpawnOpts, and TNTWithSource) and for each case calls
reg.Config().TNTWithSource(...), asserting non-nil result and that the TNT stub
was invoked; keep the existing assertions but drive inputs and expectations from
the table to remove duplication.
🪄 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: 652953a8-a857-41ed-b725-1db9f91de395
📒 Files selected for processing (28)
server/block/explosion.goserver/block/tnt.goserver/block/tnt_test.goserver/entity/damage.goserver/entity/ent.goserver/entity/projectile.goserver/entity/projectile_test.goserver/entity/register.goserver/entity/tnt.goserver/entity/tnt_test.goserver/item/register.goserver/item/shield.goserver/item/shield_test.goserver/player/player.goserver/player/shield.goserver/player/shield_test.goserver/session/entity_metadata.goserver/session/entity_metadata_test.goserver/session/handler_inventory_transaction.goserver/session/handler_inventory_transaction_test.goserver/session/handler_item_stack_request.goserver/session/handler_item_stack_request_test.goserver/session/handler_player_auth_input.goserver/session/handler_player_auth_input_test.goserver/session/world.goserver/world/entity.goserver/world/entity_test.goserver/world/sound/item.go
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
CodeRabbit follow-up:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/player/shield_test.go (2)
831-844: 💤 Low valueUse
t.Runsubtests for table-driven tests.
TestShieldDurabilityDamage,TestShouldAttemptShieldBlockBeforeHurtHandler(lines 860-907), andTestShouldAttemptShieldBlockAfterHurtHandlerWithHandlerMutatedDamage(lines 909-953) all loop over cases and uset.Fatalf, which aborts on the first failure and hides remaining cases. Promoting each case to a subtest viat.Run(test.name, ...)(or a generated name forTestShieldDurabilityDamage) gives per-case names ingo test -v/CI output and lets-runtarget a single case.♻️ Example for
TestShouldAttemptShieldBlockBeforeHurtHandler} { - if got := shouldAttemptShieldBlockBeforeHurtHandler(test.rawDamage, test.src); got != test.want { - t.Fatalf("%v: expected shouldAttemptShieldBlockBeforeHurtHandler to return %v, got %v", test.name, test.want, got) - } + test := test + t.Run(test.name, func(t *testing.T) { + if got := shouldAttemptShieldBlockBeforeHurtHandler(test.rawDamage, test.src); got != test.want { + t.Fatalf("expected shouldAttemptShieldBlockBeforeHurtHandler to return %v, got %v", test.want, got) + } + }) }🤖 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/player/shield_test.go` around lines 831 - 844, Convert the table-driven loops in TestShieldDurabilityDamage, TestShouldAttemptShieldBlockBeforeHurtHandler, and TestShouldAttemptShieldBlockAfterHurtHandlerWithHandlerMutatedDamage to t.Run subtests: give each case a stable name (either a provided "name" field in the test struct or generated via fmt.Sprintf like "%v" or "case_%d"), call t.Run(name, func(t *testing.T) { ... }) for each iteration, and replace t.Fatalf with t.Fatalf/t.Errorf inside the subtest so failures report per-case without aborting other cases; update the test case structs in those functions if needed to include a name field and ensure shieldDurabilityDamage and the two handler tests use the new subtest structure.
813-829: 💤 Low valueTighten the projectile-source assertion in
TestShieldKnocksBackMeleeAttacker.After the successful melee call at lines 817-818,
attacker.src,attacker.force, andattacker.heightare populated. The follow-up check at lines 826-828 only inspects the return value, so a regression that returnsfalsewhile still callingKnockBackfor a projectile source would silently pass. Snapshotting the fields before the second call and asserting they are unchanged would catch that.♻️ Suggested tightening
+ prevSrc, prevForce, prevHeight := attacker.src, attacker.force, attacker.height if p.knockBackShieldAttacker(entity.ProjectileDamageSource{Projectile: attacker}) { t.Fatal("expected projectile source not to knock back the attacker") } + if attacker.src != prevSrc || attacker.force != prevForce || attacker.height != prevHeight { + t.Fatalf("expected projectile source not to mutate attacker knockback state, got %v/%v/%v", attacker.src, attacker.force, attacker.height) + }🤖 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/player/shield_test.go` around lines 813 - 829, The test TestShieldKnocksBackMeleeAttacker currently checks only the boolean result for the projectile case, so a regression that still calls KnockBack would be missed; before calling p.knockBackShieldAttacker(entity.ProjectileDamageSource{Projectile: attacker}) capture attacker.src, attacker.force and attacker.height into local variables and then assert after the call that those three fields remain equal to the captured values (and still equal to the expected melee values), while also asserting the call returns false via p.knockBackShieldAttacker.
🤖 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.
Nitpick comments:
In `@server/player/shield_test.go`:
- Around line 831-844: Convert the table-driven loops in
TestShieldDurabilityDamage, TestShouldAttemptShieldBlockBeforeHurtHandler, and
TestShouldAttemptShieldBlockAfterHurtHandlerWithHandlerMutatedDamage to t.Run
subtests: give each case a stable name (either a provided "name" field in the
test struct or generated via fmt.Sprintf like "%v" or "case_%d"), call
t.Run(name, func(t *testing.T) { ... }) for each iteration, and replace t.Fatalf
with t.Fatalf/t.Errorf inside the subtest so failures report per-case without
aborting other cases; update the test case structs in those functions if needed
to include a name field and ensure shieldDurabilityDamage and the two handler
tests use the new subtest structure.
- Around line 813-829: The test TestShieldKnocksBackMeleeAttacker currently
checks only the boolean result for the projectile case, so a regression that
still calls KnockBack would be missed; before calling
p.knockBackShieldAttacker(entity.ProjectileDamageSource{Projectile: attacker})
capture attacker.src, attacker.force and attacker.height into local variables
and then assert after the call that those three fields remain equal to the
captured values (and still equal to the expected melee values), while also
asserting the call returns false via p.knockBackShieldAttacker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0556ac0-eacc-4e9f-b6e7-cf92a842e92c
📒 Files selected for processing (4)
server/entity/tnt.goserver/player/player.goserver/player/shield.goserver/player/shield_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/player/shield.go
- server/entity/tnt.go
- server/player/player.go
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 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 ac603a8. Configure here.
| } | ||
| if b, ok := e.(shieldBlocker); ok && b.ShieldBlocking() { | ||
| m.SetFlag(protocol.EntityDataKeyFlagsTwo, protocol.EntityDataFlagBlocking&63) | ||
| } |
There was a problem hiding this comment.
Shield blocking flag uses wrong metadata key or mask
Medium Severity
The shield blocking metadata flag is set using protocol.EntityDataFlagBlocking&63 on EntityDataKeyFlagsTwo. The &63 mask extracts the bit position within the second 64-bit flags field. However, the EntityDataFlagBlocking constant in the Bedrock protocol is flag index 71, and 71 & 63 = 7. If the gophertunnel library defines EntityDataFlagBlocking as the raw bit index (e.g., 7) rather than the absolute index (71), this mask operation would still yield 7 but the flag would need to be set on EntityDataKeyFlags (first field), not EntityDataKeyFlagsTwo. The crawling pattern EntityDataFlagCrawling&63 works because crawling is indeed in the second flags field. Without verifying the actual constant value, there's a risk the blocking flag is set in the wrong flags field, causing the client to not display the shield blocking animation.
Reviewed by Cursor Bugbot for commit ac603a8. Configure here.
| return false | ||
| } | ||
| return isZeroDamageProjectile(rawDamage, src) | ||
| } |
There was a problem hiding this comment.
Shield durability uses raw damage ignoring armor-reduced zero
Medium Severity
shouldAttemptShieldBlockAfterHurtHandler skips shield blocking when damageBeforeHandler (after armor/resistance reduction) is positive but damageLeft is zero (handler zeroed it). However, it also skips when armor fully reduces totalDamage to 0 and rawDamage > 0 (since damageBeforeHandler=0 and isZeroDamageProjectile fails for non-projectiles). This means melee attacks fully reduced by armor never trigger shield blocking—the shield won't play the block sound, recoil the attacker, or take durability damage. In vanilla Minecraft, shields block melee attacks regardless of armor, taking priority over armor reduction.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ac603a8. Configure here.
1 participant
Summary
Verification
go test ./...make lintgit diff --checkac603a83d68be339d4cb8842260ca41d42470c2dgo.mod,go.sum, orgophertunnelpaths changedSources
Note
Medium Risk
Touches core combat/damage flow (
Player.Hurt/Explode) plus projectile collision behaviour and TNT/explosion attribution, so regressions could affect PvE/PvP balance and knockback/damage edge cases. Changes are well-covered with extensive new tests across player, entity, block, session, and world layers.Overview
Adds a new
minecraft:shielditem (off-hand compatible, durability/repairable, registered) and implements vanilla-style shield blocking onPlayer, including a warm-up delay, front-facing source checks, durability loss, shield-disable cooldown on axe hits, recoil knockback, and shield block sound/metadata.Updates the damage and input pipelines so shield blocking is driven by Bedrock auth-input sneaking/using-item signals, stays in sync when held/offhand slots change (including inventory transactions and stack requests), and can block projectile/explosion damage even in some immunity/zero-damage cases.
Extends explosions/TNT to carry an optional
SourceandUnblockableByShieldflag, primes TNT with source+blockability via optionalEntityRegistryConfig.TNTWithSource(with fallback), persists shield-unblockable TNT via NBT, and updates projectiles to mark shield blocks and deflect (skipping hit effects/callbacks) when blocked.Reviewed by Cursor Bugbot for commit ac603a8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit